Stream: t-compiler

Topic: const-generics: literal params


Bastian Kauschke (Mar 26 2020 at 19:54, on Zulip):

I currently want to implement literals as const params without requiring a block.

This implements the following FIXME: https://github.com/rust-lang/rust/blob/2fbb07525e2f07a815e780a4268b11916248b5a9/src/librustc_parse/parser/path.rs#L440-L442

Do we want to support arbitrary paths as const params without needing blocks or only identifiers.

i.e. should this be valid

mod inner {
    const C: usize = 42;
}

struct A<const N: usize>;

type B = A<inner::C>;
Bastian Kauschke (Mar 26 2020 at 20:04, on Zulip):

cc @eddyb @varkor @boats

eddyb (Mar 26 2020 at 20:05, on Zulip):

I thought something like this was implemented already?

eddyb (Mar 26 2020 at 20:05, on Zulip):

it's in librustc_ast_lowering IIRC

eddyb (Mar 26 2020 at 20:05, on Zulip):

(but limited to single segment paths, I think?)

Bastian Kauschke (Mar 26 2020 at 20:06, on Zulip):

There was some problem, give me a second

Bastian Kauschke (Mar 26 2020 at 20:08, on Zulip):

This only works if the identifier does not exist in the type namespace: https://play.rust-lang.org/?version=nightly&mode=debug&edition=2018&gist=7853b8ddb6eba2a9b2ca1c857411776f

#![feature(const_generics)]

const C: usize = 42;
struct C;

struct A<const N: usize>;

type B = A<C>;

this fail atm

eddyb (Mar 26 2020 at 20:09, on Zulip):

I think that's intentional?

eddyb (Mar 26 2020 at 20:10, on Zulip):

not entirely sure though, I think @Vadim Petrochenkov mentored the implementation

Bastian Kauschke (Mar 26 2020 at 20:11, on Zulip):

I've seen this mentioned in some issues at least and I would be quite disappointed if this were true. Especially since it would prevent us from using unit structs in const generics

varkor (Mar 26 2020 at 20:13, on Zulip):

I think that's intentional?

it wasn't intentional, but it was suggested that fixing this would be difficult

varkor (Mar 26 2020 at 20:14, on Zulip):

let me try to remember what the problem was

varkor (Mar 26 2020 at 20:17, on Zulip):

https://github.com/rust-lang/rust/issues/67075 and https://github.com/rust-lang/rust/issues/66615 are the relevant issues

varkor (Mar 26 2020 at 20:17, on Zulip):

and have similar fixes, I imagine

varkor (Mar 26 2020 at 20:18, on Zulip):

the second issue has some more context

varkor (Mar 26 2020 at 20:18, on Zulip):

the problem is that, currently, we disambiguate between a const generic path and a type path during name resolution

varkor (Mar 26 2020 at 20:18, on Zulip):

if we find a type with that name, we treat the parameter as a type parameter; if we find a const with that name, we treat the parameter as a const parameter

varkor (Mar 26 2020 at 20:19, on Zulip):

however, if we manage to find both names, we have a bit of a problem

varkor (Mar 26 2020 at 20:19, on Zulip):

because we've got no way of telling yet what it's supposed to be

varkor (Mar 26 2020 at 20:19, on Zulip):

we won't know until type checking

varkor (Mar 26 2020 at 20:20, on Zulip):

so I suspect the way to fix this is to have a combined type/const parameter, which could be either, and then disambiguate once we get to type checking

varkor (Mar 26 2020 at 20:21, on Zulip):

cc @yodal who worked on the initial implementation

varkor (Mar 26 2020 at 20:22, on Zulip):

I would really like to see this fixed, but I don't think it'll be easy

varkor (Mar 26 2020 at 20:22, on Zulip):

maybe if you're still keen, @Bastian Kauschke, fixing the () problem could be tried first?
this is simpler, as it doesn't need name resolution at all

varkor (Mar 26 2020 at 20:23, on Zulip):

but would give you some idea of the effort involved

Bastian Kauschke (Mar 26 2020 at 20:31, on Zulip):

as keen as can be. Will look into this

centril (Mar 26 2020 at 20:41, on Zulip):

@Bastian Kauschke @varkor I'd like for https://github.com/rust-lang/rust/pull/70261 to land first before making the parser changes

centril (Mar 26 2020 at 20:41, on Zulip):

cause I'll have some ungreat merge conflicts otherwise

Bastian Kauschke (Mar 26 2020 at 20:42, on Zulip):

:thumbs_up: Won't touch the parser for now.

varkor (Mar 26 2020 at 20:42, on Zulip):

parser changes should be minimal if necessary at all, so that shouldn't be a problem

centril (Mar 26 2020 at 20:47, on Zulip):

@varkor even small changes in the middle of refactoring a function can be a headache ^^

varkor (Mar 26 2020 at 20:48, on Zulip):

sorry, I meant shouldn't be a problem to let #70261 land first

centril (Mar 26 2020 at 20:48, on Zulip):

ah :slight_smile:

Bastian Kauschke (Mar 26 2020 at 22:20, on Zulip):

hir::intravisit::Visitor uses one lifetime for both outer and inner references. https://github.com/rust-lang/rust/blob/2fbb07525e2f07a815e780a4268b11916248b5a9/src/librustc_hir/intravisit.rs#L221

This means that I can't save storage by biilding hir tys and constants on the fly :sad:
Will look into fixing this later on and just having some duplicate fields for now

Bastian Kauschke (Mar 27 2020 at 11:42, on Zulip):

Should this be forbidden?

#![feature(const_generics)]

pub struct A<T = u32, const N: usize> {
    arg: T,
}
Bastian Kauschke (Mar 27 2020 at 11:43, on Zulip):

compiles rn, even if the default arg can't actually be skipped. type B = A<7> does not work

varkor (Mar 27 2020 at 12:17, on Zulip):

hmm… I think I would expect the declaration, but also type B = A<7> to be accepted

varkor (Mar 27 2020 at 12:17, on Zulip):

it's definitely a case we should explicitly consider though; it's worth opening an issue about

Bastian Kauschke (Mar 29 2020 at 09:23, on Zulip):

e want to allow const default parameters, which afaict are planned like this: struct A<T, U = u32, const N: usize, const M: usize = 7>.

This makes using ambiguous generic args even more cumbersome.

// occupy both the ty and the value namespace.
struct BadBoi {
    field: u8,
}
const BadBoi: usize = 100;

let _: A<u8, BadBoi, 7>;
// this would end up as `Type, Ambiguous, Const`.
// Which allows for both `T ,  U, N`
// and  `T,  N, M`

zulip code blocks don't have uniform character width :unamused:

Bastian Kauschke (Mar 29 2020 at 20:10, on Zulip):

I made some decent progress here. But currently get mir failures on consts which are completely unused after type resolution.
Is there a way to not build mir for some NodeId?

eddyb (Mar 29 2020 at 20:10, on Zulip):

hmm but you want to build MIR

Bastian Kauschke (Mar 29 2020 at 20:11, on Zulip):

my current implementation works like this:
for every ambiguous generic arg, pretend that it is both a const and a type.
Once it is clear what the arg is, discard the unused interpretation.

eddyb (Mar 29 2020 at 20:12, on Zulip):

who disambiguates?

eddyb (Mar 29 2020 at 20:12, on Zulip):

if it's typeck, I don't think that's accepetable

Bastian Kauschke (Mar 29 2020 at 20:14, on Zulip):

hmm apparently librustc_ast_lowering

Bastian Kauschke (Mar 29 2020 at 20:14, on Zulip):

wait a sec

eddyb (Mar 29 2020 at 20:14, on Zulip):

that should be fine

eddyb (Mar 29 2020 at 20:14, on Zulip):

librustc_ast_lowering should be able to discard the const side if it's a type

Bastian Kauschke (Mar 29 2020 at 20:16, on Zulip):

never mind, it's not ast_lowering. I can't find the file which does the disambiguation rn. DID I ACCIDENTALLY REVERT THESE CHANGES :cry:

Bastian Kauschke (Mar 29 2020 at 20:18, on Zulip):

wtf did I do

Bastian Kauschke (Mar 29 2020 at 20:21, on Zulip):

it's librustc_typeck::astconv instead, https://github.com/rust-lang/rust/blob/285519d412ef9c65df3bcd2de2b1a3d6ca16a255/src/librustc_typeck/astconv.rs#L584-L618

Bastian Kauschke (Mar 29 2020 at 20:34, on Zulip):

Meaning thait is after typeck. Considering that we don't know what params we need before this I doubt that we can do it any other way without requiring a different syntax.

eddyb (Mar 29 2020 at 20:39, on Zulip):

that's why there are braces

eddyb (Mar 29 2020 at 20:40, on Zulip):

for arbitrary expressions

eddyb (Mar 29 2020 at 20:40, on Zulip):

anyway you should get @Vadim Petrochenkov on-board with this

eddyb (Mar 29 2020 at 20:40, on Zulip):

it may still be possible to do in AST->HIR lowering

Bastian Kauschke (Mar 29 2020 at 21:18, on Zulip):

That's disheartening, especially since this approach was previously mentioned by @varkor.
I do agree that it causes a lot of complexity for a smallish improvement, even if it would work.

Afaict at least type inference is required for this to work for all paths. Another approach would be to do this on a best effort basis
and improve diagnostics in places where it is ambiguous, which would be easier to implement, even if it is unfortunate.

Will stop working on this problem until some kind of decision is made here.

Vadim Petrochenkov (Mar 29 2020 at 22:08, on Zulip):

There's not much that can be done here before type checking.

Vadim Petrochenkov (Mar 29 2020 at 22:08, on Zulip):

(I'm talking only about paths, not additional stuff like () type vs () expr.)

Vadim Petrochenkov (Mar 29 2020 at 22:09, on Zulip):

First, https://github.com/rust-lang/rust/pull/66104#discussion_r344450671 could potentially be fixed to support disambiguating ident<Args> in addition to just ident.

Vadim Petrochenkov (Mar 29 2020 at 22:12, on Zulip):

Second, some multi-segment paths can be disambiguated, e.g. for module::Name it's clear before type checking whether Name is a type, or value, or both (assuming module is a module), but for paths potentially involving associated items we cannot disambiguate until type checking.

Vadim Petrochenkov (Mar 29 2020 at 22:13, on Zulip):

If someone prototypes the disambiguation during type checking, that would be great, I think it should be pretty realistic?

Vadim Petrochenkov (Mar 29 2020 at 22:14, on Zulip):

You'll have to keep something like two paths in HIR until then though.

eddyb (Mar 29 2020 at 22:20, on Zulip):

hmm, but you can't resolve an associated value (const/fn) outside of a body, and associated types are, well, terribly supported

varkor (Mar 29 2020 at 22:39, on Zulip):

there are some cases you can't disambiguate before type checking, because it's ambiguous

varkor (Mar 29 2020 at 22:39, on Zulip):

these are the cases I was suggesting to @Bastian Kauschke we wanted to be fixed

varkor (Mar 29 2020 at 22:40, on Zulip):

for example, if we have a const and a type with the same name, we can't distinguish between them until type checking, because we don't know which it's supposed to be

varkor (Mar 29 2020 at 22:41, on Zulip):

there are definitely some improvements we can make before type checking, e.g. the improvements @Vadim Petrochenkov suggests, but these aren't going to cover all cases

varkor (Mar 29 2020 at 22:41, on Zulip):

that's why there are braces

asking a user to use braces only if there's a type with the same name is very confusing, I think

varkor (Mar 29 2020 at 22:42, on Zulip):

especially as it isn't ambiguous

varkor (Mar 29 2020 at 22:43, on Zulip):

this would end up as Type, Ambiguous, Const.

okay, this is unfortunate

varkor (Mar 29 2020 at 22:44, on Zulip):

but this is a design problem more than an implementation problem

varkor (Mar 29 2020 at 22:44, on Zulip):

I think the sensible thing is to default to types if possible

varkor (Mar 29 2020 at 22:44, on Zulip):

e.g. we only try it as a const if we would get an error otherwise

varkor (Mar 29 2020 at 22:44, on Zulip):

this should be consistent and fairly intuitive

varkor (Mar 29 2020 at 22:44, on Zulip):

it's also an edge case

eddyb (Mar 29 2020 at 23:23, on Zulip):

yeah and {} is the override

eddyb (Mar 29 2020 at 23:24, on Zulip):

to pick the value instead

varkor (Mar 29 2020 at 23:33, on Zulip):

yes, but if it's unambiguous, I don't think you should have to override

eddyb (Mar 29 2020 at 23:55, on Zulip):

depends where the context is coming from, idk

eddyb (Mar 29 2020 at 23:56, on Zulip):

overall it's pretty finicky rn

eddyb (Mar 29 2020 at 23:56, on Zulip):

@varkor not to mention that once the WF checks are properly in place, most examples will start to fall apart regardless

eddyb (Mar 29 2020 at 23:56, on Zulip):

like e.g. Foo<{T::CONST}>

varkor (Mar 29 2020 at 23:57, on Zulip):

I'm interested in cases where we have a struct S and we want to pass the value S as a const argument

varkor (Mar 29 2020 at 23:58, on Zulip):

this is WF, but only works if we allow disambiguation during type checking

varkor (Mar 29 2020 at 23:58, on Zulip):

because otherwise we interpret it as the type S

varkor (Mar 29 2020 at 23:58, on Zulip):

(which is the same problem as accepting ())

Bastian Kauschke (Mar 30 2020 at 08:04, on Zulip):

If someone prototypes the disambiguation during type checking, that would be great, I think it should be pretty realistic?

I have a (not yet working) prototype which pretends like ambiguous args are both a type and a const and then discards the unused one after typeck.

I can try and get this to work. But I don't believe that it's worth my time if eddyb is this strongly against this.
@eddyb:

if it's typeck, I don't think that's accepetable

Bastian Kauschke (Apr 26 2020 at 20:10, on Zulip):

@varkor @Vadim Petrochenkov AST lowering happens before we start using queries queries, doesn't it :sad:

If not, we could use the same approach as https://github.com/rust-lang/rust/pull/71154 for ambiguous generic arguments,
which would be a fairly clean solution.

Bastian Kauschke (Apr 26 2020 at 20:15, on Zulip):

i.e. lower_generic_arg(arg) -> typeck_tables_of(surrounding_body)-> lower_generic_arg_with_kind(arg, ArgKind::Const/Type)

Bastian Kauschke (Apr 26 2020 at 20:16, on Zulip):

as we only need to use generic arguments during typeck once we know the corresponding context

varkor (Apr 27 2020 at 10:46, on Zulip):

as I understand it, the plan has always been to pull queries back as early as possible (I believe this was something @Zoxc was working on), but I'm not sure how far it got

Esteban Küber (Apr 27 2020 at 17:56, on Zulip):

This PR might be of interest to the people in this topic: https://github.com/rust-lang/rust/pull/71592
It starts parsing unambiguous const expressions without braces (things that start with a literal and are simple expressions). It also adds some partial recovery and suggestion for some common missing braces errors. It should inform a bit of what changes would be required to accept things like A + B, A - B and A::B() without changing anything else. It seems doable, but don't know if it is worth it to complicate the semantics to do so.

Last update: May 29 2020 at 17:55UTC