Const generic arguments should be disambiguated from type arguments

[varkor]

At the moment, const arguments have to be wrapped in curly brackets, even if they are parameters.
E.g.

impl<const X: u32> Foo<{X}> { /* ... */ }

Due to:
https://github.com/rust-lang/rust/blob/036e368d6883e0bc141c1b54578876cd5bfb2468/src/libsyntax/parse/parser.rs#L5952-L5957

Edit: Direct repro: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=85fcc01c6258677c94c5a8cd3d144cbe

[imbrem]

Do you think I could help with this? I've never contributed to rustc before, so I don't know if this is beginner difficulty...

[varkor]

@imbrem: you are welcome to try, but I think it's probably not going to be straightforward to resolve this. The general idea is to introduce a new variant to ast::GenericArg, called Ident, which will stand for either a type or a constant (because we can't distinguish them during parsing). Then, during lowering (see lowering.rs), we'll decide which it is using the extra information we have.

I think the main problem is that we do some things depending on whether a parameter represents a type or constant before lowering to the HIR, and if we don't know which of the two it is, it might make some checks difficult. I haven't tried doing this properly, so I'm not sure how much of a problem this will actually turn out to be.

(It's also possible that we don't have enough information till type-checking, so we might not be able to resolve these ambiguities until after lowering.)

Fixing this is probably going to involve some experimentation, and I don't know how hard this will be ahead-of-time. If you want to give it a try, you're welcome to, and I can try to answer questions, but there are probably easier issues to tackle (i.e. E-easy or E-mentor issues) if you just want to get started with contributing to rustc.

[imbrem]

Well, I've got an hour, so I'll give it a shot and see where it leads

[petrochenkov]

The best existing analogy to GenericArg::Ident would be PatKind::Ident that can either refer to an existing variant/constant (making it a variant/const pattern) or introduce a new variable, but we don't know what it is exactly until name resolution.

[imbrem]

After poking around in the code I have determined that the FIXME above is in fact not where #60800 errors out: in the example given, the parser actually parses N as a type a few lines below, specifically here. I verified this by putting in a debug! statement.

Any ideas how to deal with this?

[varkor]

@imbrem: this is essentially the issue (but yes, the FIXME is perhaps in a misleading place). Const parameters and type parameters are ambiguous, so whichever comes first is picked first. So instead of choosing GenericArg::Type, we need to use GenericArg::Ident (and that branch at line 5951 can be removed entirely).

(The reason it doesn't hit the const block is because can_begin_const_arg does not include Ident as a possible token.)

[imbrem]

@varkor so does that mean GenericArg::Type should be removed? I find this interesting, because on reflection (correct me if I'm wrong here) GenericArgs is one of the few (only?) places both types and values can appear in the same place.

On one hand, Rust seems to like to syntactically differentiate things (e.g. lifetimes vs types). On the other hand, I feel like from a type theoretic perspective having values and types be treated the same, at least in one context, is a lot cleaner. Could also be really nice if, in the far future, we get more dependent typing features...

[varkor]

so does that mean GenericArg::Type should be removed?

As long as there aren't any other location where we know that a generic argument is a type (and I don't think there are), yes.

On the other hand, I feel like from a type theoretic perspective having values and types be treated the same, at least in one context, is a lot cleaner.

As you say, this is entirely a syntactic ambiguity. For the most part, types and constants are treated quite differently. I don't think this is in general something we want to move towards.

[varkor]

@eddyb suggested the following variant:

GenericArg::Ident {
	ident: Ident,
	ty_res: Res,
	val_res: Res,
}

[petrochenkov]

So, I looked into this a bit more, and this feature doesn't even require extending AST structures.
What we need to do is to resolve a single-segment ast::Path in generic argument positions in two namespaces instead of one (type first, then value fallback), and then lowering to HIR can use the resolution result to disambiguate.
That's, like, ~40 lines of code in librustc_resolve/lib.rs and lowering.rs without tests.

[varkor]

@petrochenkov: if you actually have a fix for this, you're very welcome to take it — I only assigned myself to remind myself to get to it, but I don't think I'll have time very soon. Thanks for investigating, in any case!

[petrochenkov]

No, I don't have a ready fix.

[jplatte]

I'm working on this currently :)

[yodaldevoid]

@jplatte Do you have any update on your progress on this issue?

[jplatte]

No, sorry. I've been busy with another project. If you want to take over, there is some discussion about the implementation on zulip.

[yodaldevoid]

@jplatte I certainly understand getting busy with other projects. I'll see if I can't move this forward a little from the work you did.