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

in which we decline to suggest the anonymous lifetime in declarations #61679

Merged

Conversation

zackmdavis
Copy link
Member

The elided-lifetimes-in-path lint (part of our suite of Rust 2018 idiom lints which we are hoping to promote to Warn status) was firing with an illegal suggestion to write an anonymous lifetime in a
struct/item declaration (where we don't allow it). The linting code was already deciding whether to act on the basis of a ParamMode enum, indicating whether the present path-segment was part of an
expression, or anywhere else. The present case seemed to be part of the "anywhere else", and yet meriting different rules as far as the lint was concerned, so it seemed expedient to introduce a new enum member. We yank out TyKind::Path arm into its own method so that we can call it with our new ParamMode specifically when lowering struct fields—one would have hoped to think of something more elegant than this, but it definitely beats changing the signature of lower_ty to take a ParamMode!

Resolves #61124.

cc @memoryruins
r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 9, 2019
src/librustc/hir/lowering.rs Outdated Show resolved Hide resolved
if let hir::TyKind::TraitObject(..) = ty.node {
self.maybe_lint_bare_trait(t.span, t.id, qself.is_none() && path.is_global());
}
let ty = self.lower_path_ty(t, qself, path, ParamMode::Explicit, itctx);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: directly return here instead of using an intermediate variable

);
P(t)
} else {
self.lower_ty(&f.ty, ImplTraitContext::disallowed())
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively you could add a ParamMode argument to lower_ty, not sure if that is better though

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was my first idea, but I decided against it, because it seemed inelegant to change the signature when most of the TyKind match arm bodies don't concern themselves with ParamMode.

@oli-obk
Copy link
Contributor

oli-obk commented Jun 9, 2019

r=me with nits addressed

@zackmdavis zackmdavis 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, 2019
The elided-lifetimes-in-path lint (part of our suite of Rust 2018
idiom lints which we are hoping to promote to Warn status) was firing
with an illegal suggestion to write an anonymous lifetime in a
struct/item declaration (where we don't allow it). The linting code
was already deciding whether to act on the basis of a `ParamMode`
enum, indicating whether the present path-segment was part of an
expression, or anywhere else. The present case seemed to be part of
the "anywhere else", and yet meriting different rules as far as the
lint was concerned, so it seemed expedient to introduce a new enum
member. We yank out a `TyKind::Path` arm into its own method so that
we can call it with our new `ParamMode` specifically when lowering
struct fields. (The alternative strategy of changing the signature of
`lower_ty` to take a `ParamMode` would be inelegant given that most of
the `TyKind` match arm bodies therein don't concern themselves with
`ParamMode`.)

Resolves rust-lang#61124.
@zackmdavis zackmdavis force-pushed the maybe_dont_indicate_the_anonymous_lifetime branch from 9c55aa0 to 7a3184a Compare June 14, 2019 07:03
@zackmdavis
Copy link
Member Author

@bors r=oli-obk

@bors
Copy link
Contributor

bors commented Jun 14, 2019

📌 Commit 7a3184a has been approved by oli-obk

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 14, 2019
@bors
Copy link
Contributor

bors commented Jun 14, 2019

⌛ Testing commit 7a3184a with merge e699ea0...

bors added a commit that referenced this pull request Jun 14, 2019
…ifetime, r=oli-obk

in which we decline to suggest the anonymous lifetime in declarations

The elided-lifetimes-in-path lint (part of our suite of Rust 2018 idiom lints which we are hoping to promote to Warn status) was firing with an illegal suggestion to write an anonymous lifetime in a
struct/item declaration (where we don't allow it). The linting code was already deciding whether to act on the basis of a `ParamMode` enum, indicating whether the present path-segment was part of an
expression, or anywhere else. The present case seemed to be part of the "anywhere else", and yet meriting different rules as far as the lint was concerned, so it seemed expedient to introduce a new enum member. We yank out `TyKind::Path` arm into its own method so that we can call it with our new `ParamMode` specifically when lowering struct fields—one would have hoped to think of something more elegant than this, but it definitely beats changing the signature of `lower_ty` to take a `ParamMode`!

Resolves #61124.

cc @memoryruins
r? @oli-obk
@bors
Copy link
Contributor

bors commented Jun 14, 2019

☀️ Test successful - checks-travis, status-appveyor
Approved by: oli-obk
Pushing e699ea0 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 14, 2019
@bors bors merged commit 7a3184a into rust-lang:master Jun 14, 2019
@zackmdavis zackmdavis deleted the maybe_dont_indicate_the_anonymous_lifetime branch June 20, 2019 17:46
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 26, 2024
…fmease

Remove `ParamMode::ExplicitNamed`

This was introduced as a hack to improve a diagnostics suggestion in rust-lang#61679. It was subsequently broken, but also it was an incomplete hack that I don't believe we need to support, so let's just remove it.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 26, 2024
…fmease

Remove `ParamMode::ExplicitNamed`

This was introduced as a hack to improve a diagnostics suggestion in rust-lang#61679. It was subsequently broken, but also it was an incomplete hack that I don't believe we need to support, so let's just remove it.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Aug 27, 2024
Rollup merge of rust-lang#129626 - compiler-errors:explicit-named, r=fmease

Remove `ParamMode::ExplicitNamed`

This was introduced as a hack to improve a diagnostics suggestion in rust-lang#61679. It was subsequently broken, but also it was an incomplete hack that I don't believe we need to support, so let's just remove it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bad lint suggestion: '_ in type definition
5 participants