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

Lint attributes on function arguments #61856

Merged
merged 1 commit into from
Jul 29, 2019
Merged

Conversation

c410-f3r
Copy link
Contributor

@c410-f3r c410-f3r commented Jun 15, 2019

Fixes #61238.

cc #60406

@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 Jun 15, 2019
@rust-highfive

This comment has been minimized.

@oli-obk oli-obk 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 19, 2019
@@ -2,9 +2,10 @@ error[E0308]: mismatched types
--> $DIR/as-ref.rs:6:27
|
LL | opt.map(|arg| takes_ref(arg));
| --- ^^^ expected &Foo, found struct `Foo`
| |
| help: consider using `as_ref` instead: `as_ref().map`
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a regression that should be addressed before the PR can be merged. Any clue what lead to it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am investigating it

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps some parent-child relations that the suggestion relies on were broken by the new nodes in HIR?

@@ -6,7 +6,6 @@ LL | fn foo(&foo: Foo) {
|
= note: expected type `Foo`
found type `&_`
= help: did you mean `foo: &Foo`?
Copy link
Contributor

Choose a reason for hiding this comment

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

Another regression, this time in type checking

src/libsyntax/visit.rs Outdated Show resolved Hide resolved

//fn baz(#[allow(unused_mut)] mut b: i32) {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these commented out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot to include a FIXME because it is currently failing.
Still trying to figure out how to make this lint work

src/test/ui/lint/lint-unused-variables.rs Outdated Show resolved Hide resolved
src/librustc/hir/intravisit.rs Outdated Show resolved Hide resolved
@rust-highfive

This comment has been minimized.

@Centril
Copy link
Contributor

Centril commented Jul 4, 2019

I believe Oliver won't be able to review this anytime soon so... r? @petrochenkov

@rust-highfive rust-highfive assigned petrochenkov and unassigned oli-obk Jul 4, 2019
@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 Jul 4, 2019
@petrochenkov
Copy link
Contributor

The existing code looks good, but I don't know what exactly needs to be done to fix the regressions and remaining tests.

@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 Jul 5, 2019
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Jul 6, 2019

Thanks for the review, @petrochenkov.

The existing code looks good, but I don't know what exactly needs to be done to fix the regressions and remaining tests.

The biggest problem is the unused_mut lint. Someway, somehow, the DataflowResultsConsumer implementation for MirBorrowckCtxt must be modified so the used_mut attribute can be filled with the function attributes and the lint won't triggered in this line code.

@c410-f3r c410-f3r marked this pull request as ready for review July 7, 2019 12:16
@matthewjasper
Copy link
Contributor

So, to fix the unused_mut lint requires giving the parameters the correct source scopes in MIR construction. The steps to do this are:

  • Change the third field of this struct to Option<(&'tcx hir::Arg)> (

    struct ArgInfo<'tcx>(Ty<'tcx>, Option<Span>, Option<&'tcx hir::Pat>, Option<ImplicitSelfKind>);

  • Enter a new source scope inside this if let:

    if let Some(pattern) = pattern {
    let pattern = self.hir.pattern_from_hir(pattern);
    let span = pattern.span;
    match *pattern.kind {
    // Don't introduce extra copies for simple bindings
    PatternKind::Binding {
    mutability,
    var,
    mode: BindingMode::ByValue,
    subpattern: None,
    ..
    } => {
    self.local_decls[local].mutability = mutability;
    self.local_decls[local].is_user_variable =
    if let Some(kind) = self_binding {
    Some(ClearCrossCrate::Set(BindingForm::ImplicitSelf(*kind)))
    } else {
    let binding_mode = ty::BindingMode::BindByValue(mutability.into());
    Some(ClearCrossCrate::Set(BindingForm::Var(VarBindingForm {
    binding_mode,
    opt_ty_info,
    opt_match_place: Some((Some(place.clone()), span)),
    pat_span: span,
    })))
    };
    self.var_indices.insert(var, LocalsForNode::One(local));
    }
    _ => {
    scope = self.declare_bindings(
    scope,
    ast_body.span,
    &pattern,
    matches::ArmHasGuard(false),
    Some((Some(&place), span)),
    );
    unpack!(block = self.place_into_pattern(block, pattern, &place, false));
    }
    }
    }

    • The relevant code for this is here, you can extract it into a new method first. current_hir_id is the HirId of the Arg that you added, span of the pattern for the Span.

      let parent_root = tcx.maybe_lint_level_root_bounded(
      self.source_scope_local_data[source_scope].lint_root,
      self.hir.root_lint_level,
      );
      let current_root = tcx.maybe_lint_level_root_bounded(
      current_hir_id,
      self.hir.root_lint_level
      );
      if parent_root != current_root {
      self.source_scope = self.new_source_scope(
      region_scope.1.span,
      LintLevel::Explicit(current_root),
      None
      );
      }

    • At the end of the if let you should reset the source scope to its previous value.

  • Add self.local_decls[local].source_info.scope = self.source_scope; around here:

    self.local_decls[local].mutability = mutability;

@c410-f3r
Copy link
Contributor Author

ping @matthewjasper

// includes the the arg's type.
err.help(&format!("did you mean `{}: &{}`?", snippet, expected));
err.span_suggestion(
pat.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I thought that Arg had it's own Span. This needs the span of the whole Arg.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -3281,6 +3282,7 @@ impl<'a> LoweringContext<'a> {
// original argument, but with a different pattern id.
let (new_argument_pat, new_argument_id) = this.pat_ident(desugared_span, ident);
let new_argument = hir::Arg {
attrs: argument.attrs,
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I meant a test for with lint attributes.

@c410-f3r c410-f3r force-pushed the attrs-fn branch 2 times, most recently from fdd1eb6 to e553a49 Compare July 26, 2019 22:52
@@ -995,6 +997,7 @@ impl<'hir> Map<'hir> {
pub fn span(&self, hir_id: HirId) -> Span {
self.read(hir_id); // reveals span from node
match self.find_entry(hir_id).map(|entry| entry.node) {
Some(Node::Arg(arg)) => arg.pat.span,
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be arg.span now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this new commit, is the print_arg function too simple? Maybe it isn't necessary?

@matthewjasper
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 28, 2019

📌 Commit 53fc7fb has been approved by matthewjasper

@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 Jul 28, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 28, 2019
bors added a commit that referenced this pull request Jul 28, 2019
Rollup of 8 pull requests

Successful merges:

 - #61856 (Lint attributes on function arguments)
 - #62360 (Document that ManuallyDrop::drop should not called more than once)
 - #62392 (Update minifier-rs version)
 - #62871 (Explicit error message for async recursion.)
 - #62995 (Avoid ICE when suggestion span is at Eof)
 - #63053 (SystemTime docs: recommend Instant for elapsed time)
 - #63081 (tidy: Cleanup the directory whitelist)
 - #63088 (Remove anonymous_parameters from unrelated test)

Failed merges:

r? @ghost
@bors bors merged commit 53fc7fb into rust-lang:master Jul 29, 2019
@c410-f3r c410-f3r deleted the attrs-fn branch July 29, 2019 00:09
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 30, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 30, 2019
bors added a commit to rust-lang/rust-clippy that referenced this pull request Jul 30, 2019
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 30, 2019
Changes:
````
Fix breakage due to rust-lang#61856
Fix dogfood test
Hash discriminant of lifetime.name
Hash discriminant of Lifetime::Name
Updated tests.
Respond to review comments
Updated test stderr
Added doc comment fixed type printout
Respond to comments and improve printout
Responded to comments and fixed compile bug
Fixed more compile errors
Fix some of the compile errors
Changed Ty to ty, added lifetime 'tcx
Lint for type repetition in trait bounds.
````
bors added a commit that referenced this pull request Jul 31, 2019
submodules: update clippy from dc69a5c to c3e9136

Changes:
````
Fix breakage due to #61856
Fix dogfood test
Hash discriminant of lifetime.name
Hash discriminant of Lifetime::Name
Updated tests.
Respond to review comments
Updated test stderr
Added doc comment fixed type printout
Respond to comments and improve printout
Responded to comments and fixed compile bug
Fixed more compile errors
Fix some of the compile errors
Changed Ty to ty, added lifetime 'tcx
Lint for type repetition in trait bounds.
````
r? @Manishearth
bors added a commit that referenced this pull request Sep 21, 2019
Stabilize `param_attrs` in Rust 1.39.0

# Stabilization proposal

I propose that we stabilize `#![feature(param_attrs)]`.

Tracking issue: #60406
Version: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

## What is stabilized

It is now possible to add outer attributes like `#[cfg(..)]` on formal parameters of functions, closures, and function pointer types. For example:

```rust
fn len(
    #[cfg(windows)] slice: &[u16],
    #[cfg(not(windows))] slice: &[u8],
) -> usize {
    slice.len()
}
```

## What isn't stabilized

* Documentation comments like `/// Doc` on parameters.

* Code expansion of a user-defined `#[proc_macro_attribute]` macro used on parameters.

* Built-in attributes other than `cfg`, `cfg_attr`, `allow`, `warn`, `deny`, and `forbid`. Currently, only the lints `unused_variables` and `unused_mut` have effect and may be controlled on parameters.

## Motivation

The chief motivations for stabilizing `param_attrs` include:

* Finer conditional compilation with `#[cfg(..)]` and linting control of variables.

* Richer macro DSLs created by users.

* External tools and compiler internals can take advantage of the additional information that the parameters provide.

For more examples, see the [RFC][rfc motivation].

## Reference guide

In the grammar of function and function pointer, the grammar of variadic tails (`...`) and parameters are changed respectively from:

```rust
FnParam = { pat:Pat ":" }? ty:Type;
VaradicTail = "...";
```

into:

```rust
FnParam = OuterAttr* { pat:Pat ":" }? ty:Type;
VaradicTail = OuterAttr* "...";
```

The grammar of a closure parameter is changed from:

```rust
ClosureParam = pat:Pat { ":" ty:Type }?;
```

into:

```rust
ClosureParam = OuterAttr* pat:Pat { ":" ty:Type }?;
```

More generally, where there's a list of formal (value) parameters separated or terminated by `,` and delimited by `(` and `)`. Each parameter in that list may optionally be prefixed by `OuterAttr+`.

Note that in all cases, `OuterAttr*` applies to the whole parameter and not just the pattern. This distinction matters in pretty printing and in turn for macros.

## History

* On 2018-10-15, @Robbepop proposes [RFC 2565][rfc], "Attributes in formal function parameter position".

* On 2019-04-30, [RFC 2565][rfc] is merged and the tracking issue is made.

* On 2019-06-12, a partial implementation was completed. The implementation was done in [#60669][60669] by @c410-f3r and the PR was reviewed by @petrochenkov and @Centril.

* On 2019-07-29, [#61238][61238] was fixed in [#61856][61856]. The issue fixed was that lint attributes on function args had no effect. The PR was written by @c410-f3r and reviewed by @matthewjasper, @petrochenkov, and @oli-obk.

* On 2019-08-02, a bug [#63210][63210] was filed wherein the attributes on formal parameters would not be passed to macros. The issue was about forgetting to call the relevant method in `fn print_arg` in the pretty printer. In [#63212][63212], written by @Centril on 2019-08-02 and reviewed by @davidtwco, the issue aforementioned was fixed.

* This PR stabilizes `param_attrs`.

## Tests

* [On Rust 2018, attributes aren't permitted on function parameters without a pattern in trait definitions.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-2018.rs)

* [All attributes that should be allowed. This includes `cfg`, `cfg_attr`, and lints check attributes.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs)

* [Built-in attributes, which should be forbidden, e.g., `#[test]`, are.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-builtin-attrs.rs)

* [`cfg` and `cfg_attr` are properly evaluated.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs)

* [`unused_mut`](https://github.com/rust-lang/rust/blob/46f405ec4d7c6bf16fc2eaafe7541019f1da2996/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs) and [`unused_variables`](https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/lint-unused-variables.rs) are correctly applied to parameter patterns.

* [Pretty printing takes formal parameter attributes into account.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-pretty.rs)

## Possible future work

* Custom attributes inside function parameters aren't currently supported but it is something being worked on internally.

* Since documentation comments are syntactic sugar for `#[doc(...)]`, it is possible to allow literal `/// Foo` comments on function parameters.

[rfc motivation]: https://github.com/rust-lang/rfcs/blob/master/text/2565-formal-function-parameter-attributes.md#motivation
[rfc]: rust-lang/rfcs#2565
[60669]: #60669
[61856]: #61856
[63210]: #63210
[61238]: #61238
[63212]: #63212

This report is a collaborative work with @Centril.
Centril added a commit to Centril/rust that referenced this pull request Sep 21, 2019
Stabilize `param_attrs` in Rust 1.39.0

# Stabilization proposal

I propose that we stabilize `#![feature(param_attrs)]`.

Tracking issue: rust-lang#60406
Version: 1.39 (2019-09-26 => beta, 2019-11-07 => stable).

## What is stabilized

It is now possible to add outer attributes like `#[cfg(..)]` on formal parameters of functions, closures, and function pointer types. For example:

```rust
fn len(
    #[cfg(windows)] slice: &[u16],
    #[cfg(not(windows))] slice: &[u8],
) -> usize {
    slice.len()
}
```

## What isn't stabilized

* Documentation comments like `/// Doc` on parameters.

* Code expansion of a user-defined `#[proc_macro_attribute]` macro used on parameters.

* Built-in attributes other than `cfg`, `cfg_attr`, `allow`, `warn`, `deny`, and `forbid`. Currently, only the lints `unused_variables` and `unused_mut` have effect and may be controlled on parameters.

## Motivation

The chief motivations for stabilizing `param_attrs` include:

* Finer conditional compilation with `#[cfg(..)]` and linting control of variables.

* Richer macro DSLs created by users.

* External tools and compiler internals can take advantage of the additional information that the parameters provide.

For more examples, see the [RFC][rfc motivation].

## Reference guide

In the grammar of function and function pointer, the grammar of variadic tails (`...`) and parameters are changed respectively from:

```rust
FnParam = { pat:Pat ":" }? ty:Type;
VaradicTail = "...";
```

into:

```rust
FnParam = OuterAttr* { pat:Pat ":" }? ty:Type;
VaradicTail = OuterAttr* "...";
```

The grammar of a closure parameter is changed from:

```rust
ClosureParam = pat:Pat { ":" ty:Type }?;
```

into:

```rust
ClosureParam = OuterAttr* pat:Pat { ":" ty:Type }?;
```

More generally, where there's a list of formal (value) parameters separated or terminated by `,` and delimited by `(` and `)`. Each parameter in that list may optionally be prefixed by `OuterAttr+`.

Note that in all cases, `OuterAttr*` applies to the whole parameter and not just the pattern. This distinction matters in pretty printing and in turn for macros.

## History

* On 2018-10-15, @Robbepop proposes [RFC 2565][rfc], "Attributes in formal function parameter position".

* On 2019-04-30, [RFC 2565][rfc] is merged and the tracking issue is made.

* On 2019-06-12, a partial implementation was completed. The implementation was done in [rust-lang#60669][60669] by @c410-f3r and the PR was reviewed by @petrochenkov and @Centril.

* On 2019-07-29, [rust-lang#61238][61238] was fixed in [rust-lang#61856][61856]. The issue fixed was that lint attributes on function args had no effect. The PR was written by @c410-f3r and reviewed by @matthewjasper, @petrochenkov, and @oli-obk.

* On 2019-08-02, a bug [rust-lang#63210][63210] was filed wherein the attributes on formal parameters would not be passed to macros. The issue was about forgetting to call the relevant method in `fn print_arg` in the pretty printer. In [rust-lang#63212][63212], written by @Centril on 2019-08-02 and reviewed by @davidtwco, the issue aforementioned was fixed.

* This PR stabilizes `param_attrs`.

## Tests

* [On Rust 2018, attributes aren't permitted on function parameters without a pattern in trait definitions.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-2018.rs)

* [All attributes that should be allowed. This includes `cfg`, `cfg_attr`, and lints check attributes.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-allowed.rs)

* [Built-in attributes, which should be forbidden, e.g., `#[test]`, are.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-builtin-attrs.rs)

* [`cfg` and `cfg_attr` are properly evaluated.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs)

* [`unused_mut`](https://github.com/rust-lang/rust/blob/46f405ec4d7c6bf16fc2eaafe7541019f1da2996/src/test/ui/rfc-2565-param-attrs/param-attrs-cfg.rs) and [`unused_variables`](https://github.com/rust-lang/rust/blob/master/src/test/ui/lint/lint-unused-variables.rs) are correctly applied to parameter patterns.

* [Pretty printing takes formal parameter attributes into account.](https://github.com/rust-lang/rust/blob/master/src/test/ui/rfc-2565-param-attrs/param-attrs-pretty.rs)

## Possible future work

* Custom attributes inside function parameters aren't currently supported but it is something being worked on internally.

* Since documentation comments are syntactic sugar for `#[doc(...)]`, it is possible to allow literal `/// Foo` comments on function parameters.

[rfc motivation]: https://github.com/rust-lang/rfcs/blob/master/text/2565-formal-function-parameter-attributes.md#motivation
[rfc]: rust-lang/rfcs#2565
[60669]: rust-lang#60669
[61856]: rust-lang#61856
[63210]: rust-lang#63210
[61238]: rust-lang#61238
[63212]: rust-lang#63212

This report is a collaborative work with @Centril.
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 5, 2020
Changes:
````
Fix breakage due to rust-lang/rust#61856
Fix dogfood test
Hash discriminant of lifetime.name
Hash discriminant of Lifetime::Name
Updated tests.
Respond to review comments
Updated test stderr
Added doc comment fixed type printout
Respond to comments and improve printout
Responded to comments and fixed compile bug
Fixed more compile errors
Fix some of the compile errors
Changed Ty to ty, added lifetime 'tcx
Lint for type repetition in trait bounds.
````
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Lint attributes on function args have no effect
7 participants