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

`Applicability`-ify suggestions #50723

Open
zackmdavis opened this Issue May 13, 2018 · 5 comments

Comments

Projects
None yet
5 participants
@zackmdavis
Member

zackmdavis commented May 13, 2018

#50204 introduced an Applicability enum that is used to indicate whether a suggestion is suitable for non-interactive application by tools (notably rustfix), replacing a boolean that was intended for this purpose (#47540, #39254). It looks like we want to do this everywhere, but there didn't seem to be an existing issue to track this.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 13, 2018

introducing `span_suggestion_short_with_applicability`
Some would argue that this 40-character method name is ludicrously
unwieldy (even ironic), but it's the unique continuation of the
precedent set by the other suggestion methods. (And there is some hope
that someday we'll just fold `Applicability` into the signature of the
"basic" method `span_suggestion`.)

This is in support of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 13, 2018

add suggestion applicabilities to libsyntax
This is trying to be pretty conservative about marking things as
`MachineApplicable`, which is construed to be a high standard: it would
be a pretty bad user experience for a tool to apply a fix without human
input and get it wrong, whereas prompting for human Y/n feedback before
applying a `MaybeIncorrect` (but still quite likely to be correct)
suggestion is OK. In this commit, only the "`#[align = n]` →
`#[align(n)]`" and "`MyStruct { ..SameFieldsStruct, }` → `MyStruct {
..SameFieldsStruct }`" (see
rust-lang#41834 (comment) )
suggestions are awarded `MachineApplicable` status.

One suggestion label is reworded to avoid phrasing it as a question
(which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

This is in the matter of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 13, 2018

add suggestion applicabilities to librustc
We're trying to be conservative about marking things as
`MachineApplicable`: here, only the suggestions to lowercase a lint name
(when we know for a fact that the lowercase name is correct, and the
name to be replaced matches it but for case) and underscore-prefixing
unused variables, are awarded this status.

This is in the matter of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 13, 2018

add suggestion applicabilities to libsyntax
This is trying to be pretty conservative about marking things as
`MachineApplicable`, which is construed to be a high standard: it would
be a pretty bad user experience for a tool to apply a fix without human
input and get it wrong, whereas prompting for human Y/n feedback before
applying a `MaybeIncorrect` (but still quite likely to be correct)
suggestion is OK. In this commit, only the "`#[align = n]` →
`#[align(n)]`" and "`MyStruct { ..SameFieldsStruct, }` → `MyStruct {
..SameFieldsStruct }`" (see
rust-lang#41834 (comment) )
suggestions are awarded `MachineApplicable` status.

One suggestion label is reworded to avoid phrasing it as a question
(which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

This is in the matter of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 13, 2018

add suggestion applicabilities to librustc
We're trying to be conservative about marking things as
`MachineApplicable`: here, only the suggestions to lowercase a lint name
(when we know for a fact that the lowercase name is correct, and the
name to be replaced matches it but for case) and underscore-prefixing
unused variables, are awarded this status.

This is in the matter of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 20, 2018

introducing `span_suggestion_short_with_applicability`
Some would argue that this 40-character method name is ludicrously
unwieldy (even ironic), but it's the unique continuation of the
precedent set by the other suggestion methods. (And there is some hope
that someday we'll just fold `Applicability` into the signature of the
"basic" method `span_suggestion`.)

This is in support of #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 20, 2018

suggestion applicabilities for libsyntax and librustc, run-rustfix tests
Consider this a down payment on #50723. To recap, an `Applicability`
enum was recently (#50204) added, to convey to Rustfix and other tools
whether we think it's OK for them to blindly apply the suggestion, or
whether to prompt a human for guidance (because the suggestion might
contain placeholders that we can't infer, or because we think it has a
sufficiently high probability of being wrong even though it's—
presumably—right often enough to be worth emitting in the first place).

When a suggestion is marked as `MaybeIncorrect`, we try to use comments
to indicate precisely why (although there are a few places where we just
say `// speculative` because the present author's subjective judgement
balked at the idea that the suggestion has no false positives).

The `run-rustfix` directive is opporunistically set on some relevant UI
tests (and a couple tests that were in the `test/ui/suggestions`
directory, even if the suggestions didn't originate in librustc or
libsyntax). This is less trivial than it sounds, because a surprising
number of test files aren't equipped to be tested as fixed even when
they contain successfully fixable errors, because, e.g., there are more,
not-directly-related errors after fixing. Some test files need an
attribute or underscore to avoid unused warnings tripping up the "fixed
code is still producing diagnostics" check despite the fixes being
correct; this is an interesting contrast-to/inconsistency-with the
behavior of UI tests (which secretly pass `-A unused`), a behavior which
we probably ought to resolve one way or the other.

A few suggestion labels are reworded (e.g., to avoid phrasing it as a
question, which which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

zackmdavis added a commit to zackmdavis/rust that referenced this issue May 20, 2018

suggestion applicabilities for libsyntax and librustc, run-rustfix tests
Consider this a down payment on #50723. To recap, an `Applicability`
enum was recently (#50204) added, to convey to Rustfix and other tools
whether we think it's OK for them to blindly apply the suggestion, or
whether to prompt a human for guidance (because the suggestion might
contain placeholders that we can't infer, or because we think it has a
sufficiently high probability of being wrong even though it's—
presumably—right often enough to be worth emitting in the first place).

When a suggestion is marked as `MaybeIncorrect`, we try to use comments
to indicate precisely why (although there are a few places where we just
say `// speculative` because the present author's subjective judgement
balked at the idea that the suggestion has no false positives).

The `run-rustfix` directive is opporunistically set on some relevant UI
tests (and a couple tests that were in the `test/ui/suggestions`
directory, even if the suggestions didn't originate in librustc or
libsyntax). This is less trivial than it sounds, because a surprising
number of test files aren't equipped to be tested as fixed even when
they contain successfully fixable errors, because, e.g., there are more,
not-directly-related errors after fixing. Some test files need an
attribute or underscore to avoid unused warnings tripping up the "fixed
code is still producing diagnostics" check despite the fixes being
correct; this is an interesting contrast-to/inconsistency-with the
behavior of UI tests (which secretly pass `-A unused`), a behavior which
we probably ought to resolve one way or the other (filed issue #50926).

A few suggestion labels are reworded (e.g., to avoid phrasing it as a
question, which which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

bors added a commit that referenced this issue May 28, 2018

Auto merge of #50724 - zackmdavis:applicability_rush, r=Manishearth
add suggestion applicabilities to librustc and libsyntax

A down payment on #50723. Interested in feedback on whether my `MaybeIncorrect` vs. `MachineApplicable` judgement calls are well-calibrated (and that we have a consensus on what this means).

r? @Manishearth
cc @killercup @estebank

Nokel81 added a commit to Nokel81/rust that referenced this issue May 28, 2018

Make &Slice a thin pointer
Add assertions for TyS and TypeVariants sizes

Add fields to Slice

introducing `span_suggestion_short_with_applicability`

Some would argue that this 40-character method name is ludicrously
unwieldy (even ironic), but it's the unique continuation of the
precedent set by the other suggestion methods. (And there is some hope
that someday we'll just fold `Applicability` into the signature of the
"basic" method `span_suggestion`.)

This is in support of #50723.

suggestion applicabilities for libsyntax and librustc, run-rustfix tests

Consider this a down payment on #50723. To recap, an `Applicability`
enum was recently (#50204) added, to convey to Rustfix and other tools
whether we think it's OK for them to blindly apply the suggestion, or
whether to prompt a human for guidance (because the suggestion might
contain placeholders that we can't infer, or because we think it has a
sufficiently high probability of being wrong even though it's—
presumably—right often enough to be worth emitting in the first place).

When a suggestion is marked as `MaybeIncorrect`, we try to use comments
to indicate precisely why (although there are a few places where we just
say `// speculative` because the present author's subjective judgement
balked at the idea that the suggestion has no false positives).

The `run-rustfix` directive is opporunistically set on some relevant UI
tests (and a couple tests that were in the `test/ui/suggestions`
directory, even if the suggestions didn't originate in librustc or
libsyntax). This is less trivial than it sounds, because a surprising
number of test files aren't equipped to be tested as fixed even when
they contain successfully fixable errors, because, e.g., there are more,
not-directly-related errors after fixing. Some test files need an
attribute or underscore to avoid unused warnings tripping up the "fixed
code is still producing diagnostics" check despite the fixes being
correct; this is an interesting contrast-to/inconsistency-with the
behavior of UI tests (which secretly pass `-A unused`), a behavior which
we probably ought to resolve one way or the other (filed issue #50926).

A few suggestion labels are reworded (e.g., to avoid phrasing it as a
question, which which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

Initial Commit

Initial Commit

Initial Commit

Initial Commit

Change gate test to be recognizable

Correcting the lint allow

Fixed line length and renamed unstable book file

Trying a new name for the unstable book page

Changing name to use dashes

It is cx not tcx

Adding sess

Add a new test and rework the existing one to cover the correct cases

Rewrote the test with the correct headers and added a success test and some more failure tests so that the implementation works in the success case as well

Gate test had the wrong comment signature

Changing where the feature gate is coming from. Self cannot be used in a static function

Initial Commit

Initial Commit

Initial Commit

Change gate test to be recognizable

Correcting the lint allow

Fixed line length and renamed unstable book file

Trying a new name for the unstable book page

Changing name to use dashes

Add a new test and rework the existing one to cover the correct cases

Rewrote the test with the correct headers and added a success test and some more failure tests so that the implementation works in the success case as well

Gate test had the wrong comment signature

Removing disallowing attributes on 'if' statements

no message

Initial Commit

Initial Commit

Initial Commit

Change gate test to be recognizable

Correcting the lint allow

Fixed line length and renamed unstable book file

Trying a new name for the unstable book page

Changing name to use dashes

It is cx not tcx

Adding sess

Add a new test and rework the existing one to cover the correct cases

Rewrote the test with the correct headers and added a success test and some more failure tests so that the implementation works in the success case as well

Gate test had the wrong comment signature

Changing where the feature gate is coming from. Self cannot be used in a static function

Making sure that the span information is still pressent for the older error

Allowing attributes on `if` expressions

Less descructive allowing of `if` expressions

Changing compiler error for irrefutable patterns used in if-let and while-let statements to a deny warning

Initial Commit

Initial Commit

Changing compiler error for irrefutable patterns used in if-let and while-let statements to a deny warning

Initial Commit

Initial Commit

Change gate test to be recognizable

Correcting the lint allow

Fixed line length and renamed unstable book file

Trying a new name for the unstable book page

Changing name to use dashes

Updated the feature gate definition

It is cx not tcx

Adding sess

Add a new test and rework the existing one to cover the correct cases

Rewrote the test with the correct headers and added a success test and some more failure tests so that the implementation works in the success case as well

Gate test had the wrong comment signature

Changing where the feature gate is coming from. Self cannot be used in a static function

Removing disallowing attributes on 'if' statements

Making sure that the span information is still pressent for the older error

Allowing attributes on `if` expressions

Less descructive allowing of `if` expressions

Fixing allow test so that it isn't a infinite loop

Fixing the expected error messages on the tests

Correctly assosiating the comments with the correct line

Change the location of the feature-gate comment

Correct the error annotation for no-gate test

Correct error annotation for with_gate

Changing the gate test comment

Adding in the error annotation

Setting error pattern

Adding an error statement for the feature gate

Implementation of RFC 2086 - Allow Irrefutable Let patterns

author: Sebastian Malton <sebastian@malton.name>

vakaras added a commit to vakaras/rust that referenced this issue May 28, 2018

introducing `span_suggestion_short_with_applicability`
Some would argue that this 40-character method name is ludicrously
unwieldy (even ironic), but it's the unique continuation of the
precedent set by the other suggestion methods. (And there is some hope
that someday we'll just fold `Applicability` into the signature of the
"basic" method `span_suggestion`.)

This is in support of #50723.

vakaras added a commit to vakaras/rust that referenced this issue May 28, 2018

suggestion applicabilities for libsyntax and librustc, run-rustfix tests
Consider this a down payment on #50723. To recap, an `Applicability`
enum was recently (#50204) added, to convey to Rustfix and other tools
whether we think it's OK for them to blindly apply the suggestion, or
whether to prompt a human for guidance (because the suggestion might
contain placeholders that we can't infer, or because we think it has a
sufficiently high probability of being wrong even though it's—
presumably—right often enough to be worth emitting in the first place).

When a suggestion is marked as `MaybeIncorrect`, we try to use comments
to indicate precisely why (although there are a few places where we just
say `// speculative` because the present author's subjective judgement
balked at the idea that the suggestion has no false positives).

The `run-rustfix` directive is opporunistically set on some relevant UI
tests (and a couple tests that were in the `test/ui/suggestions`
directory, even if the suggestions didn't originate in librustc or
libsyntax). This is less trivial than it sounds, because a surprising
number of test files aren't equipped to be tested as fixed even when
they contain successfully fixable errors, because, e.g., there are more,
not-directly-related errors after fixing. Some test files need an
attribute or underscore to avoid unused warnings tripping up the "fixed
code is still producing diagnostics" check despite the fixes being
correct; this is an interesting contrast-to/inconsistency-with the
behavior of UI tests (which secretly pass `-A unused`), a behavior which
we probably ought to resolve one way or the other (filed issue #50926).

A few suggestion labels are reworded (e.g., to avoid phrasing it as a
question, which which is discouraged by the style guidelines listed in
`.span_suggestion`'s doc-comment).

zackmdavis added a commit to zackmdavis/this-week-in-rust that referenced this issue Aug 2, 2018

zackmdavis added a commit to zackmdavis/rust that referenced this issue Aug 2, 2018

`Applicability`-ify librustc_lint
Andrew Chin recently pointed out (rust-lang/cargo#5846) that it's
suprising that `cargo fix` (now shipping with Cargo itself!) doesn't
fix very common lint warnings, which is a good of a reminder as any
that we should finish #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue Aug 2, 2018

`Applicability`-ify librustc_lint
Andrew Chin recently pointed out (rust-lang/cargo#5846) that it's
surprising that `cargo fix` (now shipping with Cargo itself!) doesn't
fix very common lint warnings, which is as good of a reminder as any
that we should finish #50723.

zackmdavis added a commit to zackmdavis/rust that referenced this issue Aug 2, 2018

`Applicability`-ify librustc_lint
Andrew Chin recently pointed out (rust-lang/cargo#5846) that it's
surprising that `cargo fix` (now shipping with Cargo itself!) doesn't
fix very common lint warnings, which is as good of a reminder as any
that we should finish #50723.

nasa42 added a commit to cmr/this-week-in-rust that referenced this issue Aug 2, 2018

Merge pull request #685 from zackmdavis/applicability_rush
add rust-lang/rust#50723 as an easy-level call for participation
@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Aug 4, 2018

Contributor

I might have since added some span_suggestions since you started this work. We should probably take a week soonish and just plow through all cases in the codebase.

Contributor

estebank commented Aug 4, 2018

I might have since added some span_suggestions since you started this work. We should probably take a week soonish and just plow through all cases in the codebase.

kennytm added a commit to kennytm/rust that referenced this issue Aug 4, 2018

Rollup merge of #52968 - zackmdavis:app-lint-cability, r=estebank
App-lint-cability

@eminence recently pointed out (rust-lang/cargo#5846) that it's
surprising that `cargo fix` (now shipping with Cargo itself!) doesn't
fix very common lint warnings, which is as good of a reminder as any
that we should finish #50723.

(Previously, we did this on the librustc and libsyntax crates in #50724. I filed cmr/this-week-in-rust#685 in hopes of recruiting new contributors to do the rest.)

r? @estebank
@zackmdavis

This comment has been minimized.

Show comment
Hide comment
@zackmdavis

zackmdavis Aug 5, 2018

Member

We should probably take a week soonish and just plow through all cases in the codebase.

I'm still hopeful that we can recruit a reader of This Week in Rust 246 who's willing to put up with a bit of tedium in exchange for eternal open-source fame and glory.

If that's you, dear reader of this comment, here are some mentoring instructions:

  • Use your favorite code-search tool to look through the codebase for calls to span_suggestion or span_suggestion_short. (I count 79 at time of writing.)

  • For each of these (or at least some of them—this doesn't all have to happen in one pull request):

    • Change the call to span_suggestion_with_applicability or span_suggestion_short_with_applicability respectively, passing Applicability::MachineApplicable as the last argument. (You may need to put a use::errors::Applicability import at the top of the file.) That is, unless—
    • Unless you can think of a reason that the suggestion shouldn't be automatically applied by tools such as cargo fix or the Rust Language Server. If so, leave a comment and pass Applicability::MaybeIncorrect. (Examples: 1 2)
  • Open a pull request including the text "r? @estebank".

  • Bask in your eternal open-source fame and glory! 💖 👩‍🎤

Member

zackmdavis commented Aug 5, 2018

We should probably take a week soonish and just plow through all cases in the codebase.

I'm still hopeful that we can recruit a reader of This Week in Rust 246 who's willing to put up with a bit of tedium in exchange for eternal open-source fame and glory.

If that's you, dear reader of this comment, here are some mentoring instructions:

  • Use your favorite code-search tool to look through the codebase for calls to span_suggestion or span_suggestion_short. (I count 79 at time of writing.)

  • For each of these (or at least some of them—this doesn't all have to happen in one pull request):

    • Change the call to span_suggestion_with_applicability or span_suggestion_short_with_applicability respectively, passing Applicability::MachineApplicable as the last argument. (You may need to put a use::errors::Applicability import at the top of the file.) That is, unless—
    • Unless you can think of a reason that the suggestion shouldn't be automatically applied by tools such as cargo fix or the Rust Language Server. If so, leave a comment and pass Applicability::MaybeIncorrect. (Examples: 1 2)
  • Open a pull request including the text "r? @estebank".

  • Bask in your eternal open-source fame and glory! 💖 👩‍🎤

@ekse

This comment has been minimized.

Show comment
Hide comment
@ekse

ekse Aug 16, 2018

Contributor

Hi, I opened a pull request (#53418) to convert some suggestions. My goal is to convert more of them, this is my first time opening a PR for rust so I thought I'd start with something small.
Regarding formatting, is it possible to use rustfmt when working on the rust codebase? I couldn't get cargo fmt to work.

Contributor

ekse commented Aug 16, 2018

Hi, I opened a pull request (#53418) to convert some suggestions. My goal is to convert more of them, this is my first time opening a PR for rust so I thought I'd start with something small.
Regarding formatting, is it possible to use rustfmt when working on the rust codebase? I couldn't get cargo fmt to work.

@estebank

This comment has been minimized.

Show comment
Hide comment
@estebank

estebank Aug 17, 2018

Contributor

@ekse I believe it is possible, but it is a bad idea because we prefer to keep the diffs as small as possible. Unless you're actively touching the code, style only changes are slightly frowned upon. My personal rule of thumb is that if it's only affecting a few lines it's fine, if it's affecting the entire file it's not. Anything in between is a grey area that will depend on the reviewer. Keep in mind that the more extensive the diffs are, the more likely it is they will have/cause merge conflicts with other PRs.

Contributor

estebank commented Aug 17, 2018

@ekse I believe it is possible, but it is a bad idea because we prefer to keep the diffs as small as possible. Unless you're actively touching the code, style only changes are slightly frowned upon. My personal rule of thumb is that if it's only affecting a few lines it's fine, if it's affecting the entire file it's not. Anything in between is a grey area that will depend on the reviewer. Keep in mind that the more extensive the diffs are, the more likely it is they will have/cause merge conflicts with other PRs.

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 22, 2018

Rollup merge of #53504 - ekse:suggestions-applicability-2, r=estebank
Set applicability for more suggestions.

Converts a couple more calls to `span_suggestion_with_applicability`  (#50723). To be on the safe side, I marked suggestions that depend on the intent of the user or that are potentially lossy conversions as MaybeIncorrect.

r? @estebank

kennytm added a commit to kennytm/rust that referenced this issue Aug 28, 2018

Rollup merge of #53655 - jcpst:with_applicability, r=estebank
set applicability

Update a few more calls as described in #50723

r? @estebank

Mark-Simulacrum added a commit to Mark-Simulacrum/rust that referenced this issue Aug 28, 2018

Rollup merge of #53655 - jcpst:with_applicability, r=estebank
set applicability

Update a few more calls as described in #50723

r? @estebank

pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 29, 2018

Rollup merge of #53655 - jcpst:with_applicability, r=estebank
set applicability

Update a few more calls as described in #50723

r? @estebank

pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 29, 2018

Rollup merge of #53655 - jcpst:with_applicability, r=estebank
set applicability

Update a few more calls as described in #50723

r? @estebank

pietroalbini added a commit to pietroalbini/rust that referenced this issue Aug 30, 2018

Rollup merge of #53655 - jcpst:with_applicability, r=estebank
set applicability

Update a few more calls as described in #50723

r? @estebank
@Manishearth

This comment has been minimized.

Show comment
Hide comment
@Manishearth

Manishearth Sep 13, 2018

Member

Btw, I'm willing to mentor folks in doing this!

Member

Manishearth commented Sep 13, 2018

Btw, I'm willing to mentor folks in doing this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment