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

Allow qualified paths in struct construction (both expressions and patterns) #80080

Merged
merged 1 commit into from
Jun 10, 2021

Conversation

rylev
Copy link
Member

@rylev rylev commented Dec 16, 2020

Fixes #79658

@rust-highfive
Copy link
Collaborator

r? @oli-obk

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2020
@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 16, 2020

This lgtm. At this point we should probably do a cleanup (as a follow-up) by introducing some sort of ast::QPath that holds the two fields instead of working with the tuple everywhere. But that will require updates to add appropriate visitor methods and other convenience things.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 18, 2020

I have reopened #61933 which was originally fixed but is reintroduced with this PR.

Even if this is technically just a bug fix, since this is an insta-stable user-visible language change, I do believe the language team needs to sign off on it.

This PR allows the use of qualified paths in struct patterns and expressions, just like we already allow it for tuple struct patterns and expressions. As an example:

let <Bar as A>::Assoc { br: _br } = <Bar as A>::Assoc { br: 2 };

is allowed after this PR, when it wasn't allowed before.

@oli-obk oli-obk added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Dec 18, 2020
@rylev
Copy link
Member Author

rylev commented Jan 12, 2021

@nikomatsakis I believe you were going to comment on next steps for this?

@Mark-Simulacrum
Copy link
Member

I think it was me :)

We discussed this in the last few lang team meetings, and there was general agreement that this seems like the right path. However, this PR is rather loose on exact description of the change made, and it would be good to write that up (perhaps as the PR description). Specifically, a description of what was allowed before, and what is allowed now; if there's some reason to believe this case was intentionally excluded it would be good to reference the PR that did so and it's rationale as well. We were also interested in knowing if there was a specific driver for this change being made (beyond fixing the mentioned issue).

@rylev
Copy link
Member Author

rylev commented Jan 13, 2021

@Mark-Simulacrum thanks!

The question at hand is should qualified paths type aliases be useable in patterns and expressions for both struct-structs and tuple-structs. This question can be further broken down to: should they be parseable but not pass analysis, not even parseable, or fully useable?

For example, should this code which does not compile in master be made to compile (or at the very least parse)?

fn main() {
    let <WithStructStruct as Trait>::AssociatedType { a } = <WithStructStruct as Trait>::AssociatedType { a: 0 };
    let <WithTupleStruct as Trait>::AssociatedType(a) = <WithTupleStruct as Trait>::AssociatedType(0);
                                                     // ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ This is the only thing that currently parses 
    let s = StructStruct { a : 0 };
    match s {
       <WithStructStruct as Trait>::AssociatedType { a } => a,
    };

    let s = TupleStruct(0);
    match s {
       <WithTupleStruct as Trait>::AssociatedType(a) => a,
    };
}

struct StructStruct {
    a: usize
}

struct TupleStruct(usize);

trait Trait {
    type AssociatedType;
}

struct WithStructStruct;

impl Trait for WithStructStruct {
    type AssociatedType = StructStruct;
}

struct WithTupleStruct;

impl Trait for WithTupleStruct {
    type AssociatedType = TupleStruct;
}

playground link

Currently in master only tuple structs in expression position parse - though they are not allowed past analysis (rustc_resolve prevents them from being used). Everything else fails to even parse:

let  _ = <WithTupleStruct as Trait>::AssociatedType(0);
// ^ parses but does not compile

It is important to note that the use of struct-structs (but not tuple-structs) with qualified paths is allowed if that struct-struct is referred to behind an alias. For example, the following compiles already:

type Alias = <WithStructStruct as Trait>::AssociatedType;
let alias = Alias { a: 0 } ;
match alias {
  Alias { a } => a,
};

This PR allows the full use of qualified paths in struct-struct patterns and expressions and the parsing of tuple-structs in patterns. Tuple-structs are still not allowed past analysis in both expression and pattern position but this seems wrong (and was an oversight) and should be made consistent with whatever direction we decide to go with struct-structs.

Reasoning

The reason I decided to work on this was due to the need for this in generated code where the generated code knew the trait in question but not the specific associated type. Because of the nature of the associated type it was possible to construct that type but not to name it. To work around this, the alias trick above was used.

This seems like a strange inconsistency that should be resolved. Although it's not often useful, being able to refer to types through qualified paths can be useful in limited circumstances and it does seem natural and unsurprising.

@Mark-Simulacrum
Copy link
Member

Tuple-structs are still not allowed past analysis in both expression and pattern position but this seems wrong (and was an oversight) and should be made consistent with whatever direction we decide to go with struct-structs.

To clarify, is this change made in this PR? I think what you have written presents good rationale but it would be helpful to have a more bullet point summary, e.g., something like this, though the following is my hypothesis and not from reading this PR or anything:

Before this PR:

  • tuple-structs were parsed, but not resolved, with full UFCS (<Ty as Trait>::AssocType)
  • struct-structs were not parsed with full UFCS (<Ty as Trait>::AssocType)
  • struct and tuple structs were parsed and resolved with other path forms (as documented in https://doc.rust-lang.org/stable/reference/paths.html)

After this PR:

It would also be good to figure out why this restriction was added/exists - is this just by accident? Maybe @petrochenkov can provide some insight here.

@rylev
Copy link
Member Author

rylev commented Jan 14, 2021

Sorry for the confusion! Let me give it another try:

Before PR

  • Usage of a qualified path alias for structs did not parse in the following cases:
    • struct-struct in expression position
    • struct-struct in pattern position
    • tuple-struct in pattern position
  • Usage of a qualified path alias for structs parsed but was forbidden in analysis in the following cases:
    • tuple-struct in expression position
  • This means that usage of a qualified path alias for structs was not fully allowed in the language in any case.

After this PR

  • Usage of a qualified path alias for structs parses in all cases
  • Usage of qualified path alias is forbidden in analysis in the following cases:
    • tuple-struct in expression position
    • tuple-struct in pattern position
  • Usage of qualified path alias is fully allowed in the following cases:
  • struct-struct in expression position
  • struct-struct in pattern position

Work that remains to be done

As the PR currently stands there is still an inconsistency as struct-struct and tuple-structs are treated differently. This needs to be changed. I am holding off on fixing this inconsistency until we decide on how to fix the inconsistency.

I believe all these usages should at least parse so at the very least we can provide better error messages. What needs to be decided is if we should allow the user to actually use the qualified path syntax in the expression and/or pattern position.

Therefore, this PR needs to be updated to either:

  • disallow struct-struct usage in both expression and pattern position just like the PR already does not allow for tuple-structs. This does not change language semantics and will just ensure better diagnostics.
  • allow tuple-struct usage in both pattern and expression position just like the PR already does allow for struct-structs. This changes language semantics.

It was my understanding that this restriction is purely a historical accident as usage of this syntax is likely not very common as @petrochenkov points out here.

@bors

This comment has been minimized.

@JohnCSimon
Copy link
Member

merge conflicts

@JohnCSimon label: +S-waiting-on-author -S-waiting-on-author

@oli-obk
Copy link
Contributor

oli-obk commented Feb 8, 2021

@bors delegate+

please rebase and then use @bors r=oli-obk to send the PR off again

@bors
Copy link
Contributor

bors commented Feb 8, 2021

✌️ @rylev can now approve this pull request

@bors

This comment has been minimized.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2021
@oli-obk

This comment has been minimized.

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Feb 8, 2021
@rylev rylev added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 9, 2021
@petrochenkov
Copy link
Contributor

r=me with commits squashed.

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 9, 2021
@rylev rylev force-pushed the qpath-on-struct branch from f757a7e to 6936349 Compare June 10, 2021 11:24
@rylev rylev requested a review from petrochenkov June 10, 2021 11:25
@rylev rylev added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2021
@petrochenkov
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jun 10, 2021

📌 Commit 6936349 has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 10, 2021
@bors
Copy link
Contributor

bors commented Jun 10, 2021

⌛ Testing commit 6936349 with merge 16e1839...

@bors
Copy link
Contributor

bors commented Jun 10, 2021

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 16e1839 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 10, 2021
@bors bors merged commit 16e1839 into rust-lang:master Jun 10, 2021
@rustbot rustbot added this to the 1.54.0 milestone Jun 10, 2021
@programmerjake
Copy link
Member

Where's the tracking issue for this? it just points to this pull request...

@rylev rylev deleted the qpath-on-struct branch July 6, 2021 19:56
@rylev
Copy link
Member Author

rylev commented Jul 6, 2021

@programmerjake there was no tracking issue for this. I will make one, and change where the feature gate points to.

m-ou-se added a commit to m-ou-se/rust that referenced this pull request Jul 9, 2021
…, r=nagisa

Change linked tracking issue for more_qualified_paths

This updates the linked tracking issue for the `more_qualified_paths` feature from the implementation PR rust-lang#80080 to an actual tracking issue rust-lang#86935.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Parsable associated enum constructor