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

syntax: Remove warning for unnecessary path disambiguators #57565

Merged
merged 1 commit into from
Mar 28, 2019

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Jan 13, 2019

rustfmt is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in #43540 (where it was introduced).
One hardcoded warning less.

Closes #58055

r? @nikomatsakis

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2019
@Centril Centril added I-nominated T-lang Relevant to the language team, which will review and decide on the PR/issue. labels Jan 13, 2019
@nikomatsakis
Copy link
Contributor

@rfcbot fcp merge

There used to be a hard-coded warning for unnecessary :: in a path (e.g., Vec::<u32> as a type). As @petrochenkov notes, this is no longer needed, since rustfmt will silently normalize this. In general, I favor using rustfmt over warnings to try and push minor stylistic nits -- far less annoying. But in this particular case, it would also be good, because this warning appears so early we can't easily use the standard lint system, so it cannot be "allowed" or "denied" etc. Therefore, I am in favor of merging this PR and removing the warning.

@rfcbot
Copy link

rfcbot commented Jan 15, 2019

Team member @nikomatsakis has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jan 15, 2019
@Centril
Copy link
Contributor

Centril commented Jan 15, 2019

I feel that this style is distinctly unidiomatic and would therefore fit well into nonstandard_style; however, if that is exceedingly difficult to do well in the compiler (e.g. it would slow things down or incur very ugly hacks) I could be swayed to not lint anymore. Feels like a good topic to discuss on Thursday.

@nikomatsakis
Copy link
Contributor

@petrochenkov question -- can we set a flag in the resulting AST to indicate that an unnecessary :: was present, in which case we could make a "true lint", right?

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Jan 17, 2019

I'm still happy for rustfmt to silently normalize, but @cramertj feels like there is value in having a "teachable moment" here -- but it'd be nice if it were rustfix'able or something like that. I can certainly see that.

@petrochenkov
Copy link
Contributor Author

can we set a flag in the resulting AST to indicate that an unnecessary :: was present

Ugh, we can but that would at least increase the size of every path segment in AST.

The hardcoded warning can be converted into a lint without extending AST, but it will be attached to the crate root and only could be allowed/denyed at crate granularity.

@nikomatsakis
Copy link
Contributor

The hardcoded warning can be converted into a lint without extending AST, but it will be attached to the crate root and only could be allowed/denyed at crate granularity.

Sure, but that seems pretty surprising. I guess it could be considered a long-standing bug.

Presumably we could do something where we record the span, too, and try to reproduce the node-id from that.

@nikomatsakis
Copy link
Contributor

To be clear, I don't know that it's worth the effort.

@nikomatsakis
Copy link
Contributor

Particularly not if we adopt the "remove turbofish" RFC, in which case... nobody would likely think to add :: in the first place.

@bors

This comment has been minimized.

@rfcbot
Copy link

rfcbot commented Jan 26, 2019

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 26, 2019
@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Feb 5, 2019
@rfcbot
Copy link

rfcbot commented Feb 5, 2019

The final comment period, with a disposition to merge, as per the review above, is now complete.

@Dylan-DPC-zz
Copy link

ping from triage @petrochenkov you have a conflict to resolve

@Dylan-DPC-zz Dylan-DPC-zz 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 Mar 4, 2019
@petrochenkov
Copy link
Contributor Author

Rebased.

@petrochenkov petrochenkov 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 Mar 4, 2019
@bors

This comment has been minimized.

@Dylan-DPC-zz
Copy link

ping from triage @petrochenkov more conflicts :D (maybe ask for a r+ before more conflicts arise)

@petrochenkov
Copy link
Contributor Author

@Dylan-DPC
The S-waiting-on-review label is already asking for r+.
(Also, conflicts don't usually prevent reviewing, that's why rfcbot doesn't reset the label to S-waiting-on-author when they happen, so the triage team probably shouldn't do it either.)

@Dylan-DPC-zz
Copy link

@petrochenkov true though, but i have seen some reviewers wait for conflicts to be resolved before reviewing (especially if the conflict relates to the change being made).

Yeah i know the label is for that, but sometimes reviewers oversee that and won't check a PR until they are pinged or there is a discussion somewhere.

@bors

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Mar 26, 2019

Reassigning to myself to lessen Niko's review load; r? @Centril

@bors r+

@bors
Copy link
Contributor

bors commented Mar 26, 2019

📌 Commit 4b38294 has been approved by Centril

@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 Mar 26, 2019
Centril added a commit to Centril/rust that referenced this pull request Mar 27, 2019
syntax: Remove warning for unnecessary path disambiguators

`rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced).
One hardcoded warning less.

Closes rust-lang#58055

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Mar 27, 2019
syntax: Remove warning for unnecessary path disambiguators

`rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced).
One hardcoded warning less.

Closes rust-lang#58055

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Mar 27, 2019
syntax: Remove warning for unnecessary path disambiguators

`rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced).
One hardcoded warning less.

Closes rust-lang#58055

r? @nikomatsakis
cuviper added a commit to cuviper/rust that referenced this pull request Mar 28, 2019
syntax: Remove warning for unnecessary path disambiguators

`rustfmt` is now stable and it removes unnecessary turbofishes, so removing the warning as discussed in rust-lang#43540 (where it was introduced).
One hardcoded warning less.

Closes rust-lang#58055

r? @nikomatsakis
bors added a commit that referenced this pull request Mar 28, 2019
Rollup of 18 pull requests

Successful merges:

 - #57293 (Make some lints incremental)
 - #57565 (syntax: Remove warning for unnecessary path disambiguators)
 - #58253 (librustc_driver => 2018)
 - #58837 (librustc_interface => 2018)
 - #59268 (Add suggestion to use `&*var` when `&str: From<String>` is expected)
 - #59283 (Make ASCII case conversions more than 4× faster)
 - #59284 (adjust MaybeUninit API to discussions)
 - #59372 (add rustfix-able suggestions to trim_{left,right} deprecations)
 - #59390 (Make `ptr::eq` documentation mention fat-pointer behavior)
 - #59393 (Refactor tuple comparison tests)
 - #59420 ([CI] record docker image info for reuse)
 - #59421 (Reject integer suffix when tuple indexing)
 - #59430 (Renames `EvalContext` to `InterpretCx`)
 - #59439 (Generalize diagnostic for `x = y` where `bool` is the expected type)
 - #59449 (fix: Make incremental artifact deletion more robust)
 - #59451 (Add `Default` to `std::alloc::System`)
 - #59459 (Add some tests)
 - #59460 (Include id in Thread's Debug implementation)

Failed merges:

r? @ghost
@bors bors merged commit 4b38294 into rust-lang:master Mar 28, 2019
@petrochenkov petrochenkov deleted the turbowarn branch June 5, 2019 16:33
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. 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.

7 participants