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

Dealing with type/const ambiguities #480

Closed
1 of 3 tasks
lcnr opened this issue Jan 19, 2022 · 1 comment
Closed
1 of 3 tasks

Dealing with type/const ambiguities #480

lcnr opened this issue Jan 19, 2022 · 1 comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team

Comments

@lcnr
Copy link
Contributor

lcnr commented Jan 19, 2022

Proposal

It is not always possible to check whether a given generic argument should be a type or a const until typeck. While this is mostly fine, this most makes it difficult to represent ambiguous paths and _ in the hir.

Examples

This is the behavior on stable:

// When encountering ambiguity, we prefer types over constants,
// requiring the use of braces `{ N }` to interpret
// things as constants in case an identifier exists in
// both the type and the value namespace.
//
// `_` is forbidden as a const argument for now.
enum N { Variant };
struct Foo<const N: usize>;
fn foo<const N: usize>() -> Foo<N> {
    //~^ ERROR type provided when a constant was expected
    Foo
}

const N: usize = 3;
type Bar = Foo<N>; 
//~^ ERROR type provied when a constant was expected

fn main() {
    let _: Foo<3> = foo::<_>();
    //~^ ERROR type provided when a constant was expected
}

As we want to be able to resolve that ambiguity and get the above example to compile, we need a way to represent ambiguous generic arguments in the hir (and to a lesser degree in the ast). This representation should ideally avoid multiple equivalent representation and minimize the risk of misuse.

This MCP only cares about the hir for now as we don't really use the ast for anything which cares about types or constants.

Suggested solution

Modify the hir AST to the following

enum SharedTyConst<'hir> {
    Infer(...), // `_`
    Path(...),
}

enum TyKind<'hir> {
    Slice(&'hir Ty<'hir>),
    Array(&'hir Ty<'hir>, ArrayLen),
    // ...all existing variants, except for `Infer` and `Path`
}

enum ArrayLen<'hir> {
    Shared(SharedTyConst<'hir>),
    Body(AnonConst),
}

// Probably need a different name for this
struct Ty<'hir> {
    hir_id: HirId,
    kind: TyKind<'hir>,
    span: Span,
}

// This one is new, for type not directly used as a generic arg.
enum BikeshedTy {
    Shared(SharedTyConst<'hir>),
    Type(Ty<'hir>),
}

pub enum GenericArg<'hir> {
    Lifetime(Lifetime),
    Type(Ty<'hir>),
     // ConstArg is `AnonConst + Span`, mostly equiv to `ArrayLen::Body`.
    Const(ConstArg),
    Ambiguous(SharedTyConst<'hir>),
}

With this setup, every type and constant can only be represented one way.

How hir::intravisit::Visitor should now be implemented to avoid bugs while still being fairly usable.

enum SharedContext {
    FreeType,
    GenericArg,
    ArrayLength,
}

trait Visitor {
    fn visit_ty(&mut self, t: &'v Ty<'v>);
    fn visit_anon_const(&mut self, ct: AnonConst);
    fn visit_shared_ty_const(
        &mut self,
        shared: SharedTyConst<'hir>,
        context: SharedContext,
    )
    
    // The rest remains the same.
}

// calls `visit_ty` or `visit_shared_ty_const`.
fn walk_bikeshed_ty(visitor, bikeshed_ty) { ... }
fn walk_array_len(visitor, array_len) { ... }

While we need to add comments to visit_ty on how to deal with _ and paths, you can't really misuse that as these variants just don't exist.

Another issue is what happens if we want to get a ty::Ty for some hir::Ty. Afaict the only hir visitor which does that is HirWfCheck which I intend to manually update to also consider SharedTyConst. I am not sure if there's a good way to prevent us from forgetting this in the general case.

Sidenote: Paths and nameres

Not yet sure how to best deal with the ambiguity for paths during nameres, maybe by adding a variant Res::ForGenericArg(Box<[Res; 2]>) or something? 🤷

Mentors or Reviewers

If you have a reviewer or mentor in mind for this work, mention then
here. You can put your own name here if you are planning to mentor the
work.

Process

The main points of the Major Change Process are as follows:

  • File an issue describing the proposal.
  • A compiler team member or contributor who is knowledgeable in the area can second by writing @rustbot second.
    • Finding a "second" suffices for internal changes. If however, you are proposing a new public-facing feature, such as a -C flag, then full team check-off is required.
    • Compiler team members can initiate a check-off via @rfcbot fcp merge on either the MCP or the PR.
  • Once an MCP is seconded, the Final Comment Period begins. If no objections are raised after 10 days, the MCP is considered approved.

You can read more about Major Change Proposals on forge.

Comments

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

@lcnr lcnr added T-compiler Add this label so rfcbot knows to poll the compiler team major-change A proposal to make a major change to rustc labels Jan 19, 2022
@rustbot
Copy link
Collaborator

rustbot commented Jan 19, 2022

This issue is not meant to be used for technical discussion. There is a Zulip stream for that. Use this issue to leave procedural comments, such as volunteering to review, indicating that you second the proposal (or third, etc), or raising a concern that you would like to be addressed.

cc @rust-lang/compiler @rust-lang/compiler-contributors

@rustbot rustbot added the to-announce Announce this issue on triage meeting label Jan 19, 2022
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Jan 20, 2022
@lcnr lcnr closed this as completed Jul 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
major-change A proposal to make a major change to rustc T-compiler Add this label so rfcbot knows to poll the compiler team
Projects
None yet
Development

No branches or pull requests

3 participants