Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

We shouldn't even try to resolve irrefutable patterns as constants #49680

Closed
Manishearth opened this issue Apr 5, 2018 · 24 comments
Closed

We shouldn't even try to resolve irrefutable patterns as constants #49680

Manishearth opened this issue Apr 5, 2018 · 24 comments
Labels
A-resolve Area: Name resolution C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@Manishearth
Copy link
Member

pub const bar: u8 = 1;
fn foo(bar: bool) { }

gives me a type error, because it resolves bar to the constant and then realizes it's the wrong type.

Resolving bar to a constant isn't something that will ever work anyway, because if you write something like

pub const bar: bool = true;
fn foo(bar: bool) { }

it will move on to the "refutable pattern in function argument" error.

We should just not attempt constant resolution for irrefutable patterns.

This is technically a language change -- code that would previously not compile will now compile. cc @rust-lang/lang

@Manishearth
Copy link
Member Author

Really given this and #33118 (comment) I'm half convinced we should just epoch-break const patterns to have an explicit const instead.

@eddyb
Copy link
Member

eddyb commented Apr 5, 2018

Note the difference between unhygienic old macros and hygienic new macros.
EDIT: this comment should probably have been made on #49679 instead.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2018

See #45050 and linked threads/issues.

The proper solution is to

Fixing #33118 (comment) is in my queue, but I didn't consider it high-priority because derives successfully worked for years with their little underscore mangling tricks.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2018

One horrible workaround that arguably relies on bug #33118 (comment) (please don' t use it)

const bar: u8 = 10;

fn main() {
    fn bar() {} // fn bar shadows const bar
    let bar = (); // OK, bar resolves to fn, fns can't be patterns
}

@Manishearth
Copy link
Member Author

@petrochenkov Are you commenting on the correct issue? This one doesn't have to do with macros; it was found by attempting to fix a macro issue but really it's its own thing.

The question is, macros aside, should irrefutable patterns allow binding on constants at all?

@petrochenkov
Copy link
Contributor

Are you commenting on the correct issue?

Yes.

The question is, macros aside, should irrefutable patterns allow binding on constants at all?

The previous decision was "Yes".

@Manishearth
Copy link
Member Author

That ... seems to be deciding something else, that happened to impact this situation as well -- do we have reasoning for why this should be the case for constants?

@kennytm kennytm added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-feature-request Category: A feature request, i.e: not implemented / a PR. labels Apr 5, 2018
@petrochenkov
Copy link
Contributor

Ah, now I see what is the source of confusion.
Constructors of unit structs and variants are basically constants of their respective types - they are treated equivalently in patterns and expressions.

@Manishearth
Copy link
Member Author

Manishearth commented Apr 5, 2018

We could remodel it such that when you use such a variant or struct in a pattern it's matched as a variant, not a constant, and then disallow constants in irrefutable patterns. Sounds like it may require not insignificant refactoring.

IOW, this seems to be a limitation imposed by the current implementation of pattern matching, and not a limitation of language design.

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2018

Yes, but why discern between them?
If you write let x = 10; you can still hit a type mismatch with random struct x; or enum E { x } even if constants specifically are not treated as patterns.

@Manishearth
Copy link
Member Author

Because there the ambiguity makes sense -- you may indeed want to match on x the struct (I mean, it's useless in this case, but in a more complex pattern less so).

OTOH here there's no reason to do so, because it's going to error at a later stage anyway. The fact that the compiler errors early prevents it from falling back to a binding. There's no real ambiguity here, whereas there is in the struct x; case.

@Manishearth
Copy link
Member Author

From the point of view of how this is implemented the behavior seems logical, but from the typical pattern mental model the current behavior seems very surprising.

@scottmcm
Copy link
Member

scottmcm commented Apr 5, 2018

I'm half convinced we should just epoch-break const patterns to have an explicit const instead.

Sounds great. There are so many traps with const-like things in patterns.

bikeshed: const generics uses {} to put an expression that must be const in a type; perhaps patterns could use the same? match foo { {1+2}..={N} => ... }

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2018

@Manishearth

because it's going to error at a later stage anyway.

Nope!

#[derive(PartialEq, Eq)]
struct S;

const C: S = S;

fn main() {
    let C = S; // OK
}

I thought about this issue more than once when (re)writing the pattern resolution code and in the end my position was that 1) in cases when it really matters and you can have conflicts with code you don't control (macros) the proper solution is macro hygiene 2) in all other cases it doesn't really matter so let's use the most simple and consistent possible rules (aka always disambiguate in favor of patterns everywhere).

@scottmcm
Curiously, the const generic RFC currently specifies that const generics do the same kind of resolution-based (not syntax-based) disambiguation as patterns.

struct Array<U, const N> { ... }

type A = Array<U, N>; // OK without braces if N resolves to a value

@Manishearth
Copy link
Member Author

I'll note that in the example you give it makes no difference whatesoever if C resolved to the constant or a new binding, and it will continue to work the same way if this change is made 😄

@petrochenkov
Copy link
Contributor

petrochenkov commented Apr 5, 2018

@Manishearth
That's until S has a destructor 😄

#[derive(PartialEq, Eq)]
struct S;

const C: S = S;

impl Drop for S {
    fn drop(&mut self) {
        println!("BOOM");
    }
}

fn main() {
    println!("barrier 1");
    let C = S;
    println!("barrier 2");
    let fresh = S;
    println!("barrier 3");
}

-------------------
barrier 1
BOOM
barrier 2
barrier 3
BOOM

@Manishearth
Copy link
Member Author

Manishearth commented Apr 5, 2018 via email

@petrochenkov
Copy link
Contributor

One more reason to keep the status quo rules is that refutability can potentially change in the future.
If let syntax is reused for matching expressions in some variation of rust-lang/rfcs#929 (not the syntax I'd prefer, but still possible), then let CONST = EXPR becomes a very much reasonable interpretation.

@nikomatsakis
Copy link
Contributor

Some points to consider:

  • First off, I don't like the idea of pattern syntax depending on whether we are in a refutable or irrefutable context. That just seems confusing to me.
  • Second, the main reason we opted for the current "DWIM" handling is not constants, but rather nullary variants like None, which have all the same issues
    • There is no way we are going to make you change the None pattern to const None
    • Historical note: before we made that change, you used to have to write None. to match a constant
  • If we were going to change anything, I'd prefer to have us distinguish bindings (e.g., via the keyword let)
    • but I'd say that's water under the bridge at this point

That said, could somebody back up for a second and summarize the background here? That is, what are the issues I should be reading into? :)

@Manishearth
Copy link
Member Author

There is no way we are going to make you change the None pattern to const None

I'm not suggesting this -- variants and struct ctors are never ambiguous. The only things that are actually ambiguous are consts (not things internally considered const by the compiler, actual consts).

That said, could somebody back up for a second and summarize the background here? That is, what are the issues I should be reading into? :)

I actually have a queued up blog post about this interesting rabbit-hole, otherwise I think #45050, #45050 (comment) , and #33118 (comment) affect this.

@joshtriplett
Copy link
Member

joshtriplett commented Apr 9, 2018

I'm curious how many people regularly encounter this construct, that in fn foo(bar: bool), bar can be a pattern rather than a name? Now that I know to look for it, I found it documented at https://doc.rust-lang.org/stable/book/second-edition/ch18-01-all-the-places-for-patterns.html#function-parameters.

Still, under what circumstances does it make sense to use a constant in such a pattern? As far as I can tell, any time you do that, you'll either get an error (in which case this just becomes "how do we give a clearer error?"), or you have a parameter whose type is an enum with one variant with no arguments, a name that matches that one variant or a constant containing it, and a non-Rustic naming convention for either the variant or the function parameter (in which case, why pass it around at all?).

So, rather than trying to come up with a completely general solution like "let's ban constants in irrefutable patterns", could we just treat this as "in cases where we know we'll error out anyway, can we give a better error message for the common case"? Help people realize that they have a name conflict, and that that turns out to matter because of a language feature (patterns in function parameters) that they might not even know about (so the error message shouldn't assume they do).

@Manishearth
Copy link
Member Author

Right, that's my point -- constants are useless in irrefutable patterns because they will always error (except for when you have a unit typed constant as petrochenkov mentioned, though in this case it's basically irrelevant unless you care about destruction order).

So, rather than trying to come up with a completely general solution like "let's ban constants in irrefutable patterns"

We already ban constants in irrefutable patterns, it's just that the way we ban them also ends up blocking some useful syntax. Instead of banning them outright, we should treat these things as bindings.

A better error message would be nice too, but I feel like we can reduce the number of error cases entirely here.

@Manishearth
Copy link
Member Author

To be clear: What I'm suggesting here is to make more things compile, not less -- it's not a breaking change except for the unit typed destructor case petrochenkov mentioned (and it's questionable if that's a breaking change at all)

@Manishearth
Copy link
Member Author

Oh, also, it occurred to me: even if we go through with this; we still have to deal with the fact that bindings can't shadow consts anyway (#33118 (comment)). This fix is akin to using foo @ _ in place of foo in irrefutable patterns where foo is already a constant; however the following code:

const foo: u8 = 1;
let foo @ _ = 2;

doesn't compile anyway because the binding shadows a constant.

If we fix #33118 we have some chance of fixing this, but till then, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-resolve Area: Name resolution C-feature-request Category: A feature request, i.e: not implemented / a PR. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

7 participants