Check for known but incorrect attributes by tejom · Pull Request #49291 · rust-lang/rust

tejom · 2018-03-23T03:23:38Z

Change nested_visit_map so it will recursively check functions
Add visit_stmt and visit_expr for impl Visitor for CheckAttrVisitor and check for incorrect
inline and repr attributes on staements and expressions
Add regression test for issue Known but incorrect attributes over statements/expressions are ignored. #43988

rust-highfive · 2018-03-23T03:23:43Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @petrochenkov (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

tejom · 2018-03-23T03:25:02Z

r? @petrochenkov

@abonander

bors · 2018-03-23T21:49:27Z

☔ The latest upstream changes (presumably #49308) made this pull request unmergeable. Please resolve the merge conflicts.

abonander · 2018-03-23T21:56:04Z

@tejom so that PR was a rollup which added code for another attribute to CheckAttrVisitor: https://github.com/rust-lang/rust/pull/49308/files#diff-a7a4db27e720b5572001d15719153b54

The preferred approach is to rebase on top of master (git rebase upstream/master where upstream is rust-lang/rust) and then resolve the merge conflict that comes up.

tejom · 2018-03-24T00:11:10Z

Alright cool. The commit doesn't look to crazy to deal with. Thanks!

petrochenkov · 2018-03-26T00:01:09Z

I don't understand what is the logic here.
How attributes on #[attr] my_fn_call() could be handles by items or statements?

I think I got confused reading the comments on the Expr_ enum. I think only ExprAssign overlaps with statements then.
let a = 1 seems to be both an expression and statement?
I can take this section out and everything seems to work fine still. The error that appears for
#[repr]let _y = "123";
is attribute should not be applied to statements with and without this match.

let a = 1 is not an expression, ExprAssign is a = b.
If you have a statement with one expression, like
a = b;
then it's represented roughly like StmtExpr(ExprAssign) and attributes are attached to the outer statement and not inner expression.

Oh Ok. I see. Thanks for the clarification.

petrochenkov · 2018-03-26T00:01:45Z

LGTM beside one comment (also needs rebase).

- Change nested_visit_map so it will recusively check functions - Add visit_stmt and visit_expr for impl Visitor for CheckAttrVisitor and check for incorrect inline and repr attributes on staements and expressions - Add regression test for isssue rust-lang#43988

petrochenkov · 2018-03-27T08:18:58Z

 attr.span,
 stmt.span,
- &format!("attribute should not be applied to statements"),
+ &format!("attribute should not be applied a statement"),


Missing "to".

petrochenkov · 2018-03-27T08:20:42Z

+
+ #[inline(ABC)]
+ foo();
+ //~^^ ERROR attribute should be applied to function


All the tests are for statements, could you add a test for an expression too?
Something like let x = #[repr] y;

I added this example. I needed to add a feature in the tests to get it to actually run.
Otherwise error[E0658]: attributes on non-item statements and expressions are experimental. (see issue #15701) stopped all of the tests early.

petrochenkov · 2018-03-27T08:21:29Z

r=me after addressing comments

petrochenkov · 2018-03-27T17:02:26Z

@bors r+

bors · 2018-03-27T17:02:27Z

📌 Commit 4957a40 has been approved by petrochenkov

kennytm · 2018-03-28T22:06:11Z

@bors p=3

bors · 2018-03-29T03:13:17Z

⌛ Testing commit 4957a40 with merge 3615093...

…, r=petrochenkov Check for known but incorrect attributes fixes#43988 - Change nested_visit_map so it will recursively check functions - Add visit_stmt and visit_expr for impl Visitor for CheckAttrVisitor and check for incorrect inline and repr attributes on staements and expressions - Add regression test for issue #43988

bors · 2018-03-29T05:44:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing 3615093 to master...

kivikakk · 2018-03-31T10:12:37Z

Hey there, I'm investigating pest-parser/pest#218, where pest fails to build on nightly 2018-03-29; it looks like this merge is the change that did it.

I'm not sure exactly why, but one suspicion is that #[inline] as applied to a lambda is now being disallowed? Take this example:

fnmain(){#[inline] || {};}

This compiles without any warning or error on e5277c1 2018-03-28, but on ae544ee 2018-03-29:

$ rustc test.rserror[E0518]: attribute should be applied to function --> test.rs:2:2 |2 | #[inline] || { }; | ^^^^^^^^^ ------ not a functionerror: aborting due to previous errorFor more information about this error, try `rustc --explain E0518`.

Is this intentional?

petrochenkov · 2018-03-31T10:21:42Z

Is this intentional?

Yes, #[inline] attribute never had any effect on lambdas.
(IIRC, operator () on lambdas is automatically marked as inline by the compiler.)

kivikakk · 2018-03-31T10:22:25Z

Good to know, thank you! ❤️

Amanieu · 2018-04-03T03:16:59Z

Actually, #[inline(always)]does have an effect on closures. This doesn't work any more and I have some code that relies on inline for optimizations.

Amanieu · 2018-04-03T03:18:35Z

The generated LLVM IR (in debug builds) for this example is different if you remove #[inline(always)]:

#![feature(stmt_expr_attributes)]#![crate_type="rlib"]pubfnmain(){let x = #[inline(always)] || {};x();}

tejom · 2018-04-03T04:49:58Z

It looks like the code generated for your example with -O option is the same for both? That's what I saw with the stable build.

Amanieu · 2018-04-03T04:53:05Z

That's because the closure is inlined anyways in release builds. You only notice the difference in debug builds where inline hint are ignored unless they are forced.

My point is that #[inline(always)]does have an effect on code generation when applied to closures, and this change is a regression since it now disallows this (though technically not a break change since stmt_expr_attributes is still unstable anyways).

tejom · 2018-04-04T01:07:07Z

I'm just asking because I'm curious, would this be bug with the compiler not inlining closures when not using one of the higher optimization levels? Seems like that is what the expected behavior was?

Amanieu · 2018-04-04T01:10:10Z

The inlining works fine. The bug is that your PR is too strict: #[inline] should be allowed on closures as well as functions.

rust-highfive assigned petrochenkov Mar 23, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 23, 2018

petrochenkov reviewed Mar 26, 2018
View reviewed changes

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 Mar 26, 2018

tejom added 2 commits March 26, 2018 08:43

Remove an unnecessary/incorrect match in the expression check function
48825bc

tejom force-pushed the check-for-known-but-incorrect-attributes branch from 7bf1158 to 48825bcCompare March 27, 2018 02:49

petrochenkov reviewed Mar 27, 2018
View reviewed changes

Add extra test for expressions and fix typo in message
4957a40

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 Mar 27, 2018

bors merged commit 4957a40 into rust-lang:masterMar 29, 2018

bors mentioned this pull request Mar 29, 2018
RFC 2008 non-exhaustive enums/structs: Finishing Touches #49345
Merged

kivikakk mentioned this pull request Mar 31, 2018
FTBFS on 1.26.0-nightly (80785a547 2018-03-30) pest-parser/pest#218
Closed

kivikakk mentioned this pull request Mar 31, 2018
remove #[inline] on lambdas pest-parser/pest#219
Merged

Amanieu mentioned this pull request Apr 4, 2018
#[inline] is no longer accepted on closures #49632
Closed

alexcrichton mentioned this pull request Apr 16, 2018
Regression in 1.26 with misplaced attributes #49934
Closed

Uh oh!

Uh oh!

Conversation

tejom commented Mar 23, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented Mar 23, 2018

Uh oh!

tejom commented Mar 23, 2018

Uh oh!

bors commented Mar 23, 2018

Uh oh!

abonander commented Mar 23, 2018

Uh oh!

tejom commented Mar 24, 2018

Uh oh!

petrochenkovMar 26, 2018

Choose a reason for hiding this comment

Uh oh!

tejomMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkovMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

tejomMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Mar 26, 2018

Uh oh!

petrochenkovMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkovMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

tejomMar 27, 2018

Choose a reason for hiding this comment

Uh oh!

petrochenkov commented Mar 27, 2018

Uh oh!

petrochenkov commented Mar 27, 2018

Uh oh!

bors commented Mar 27, 2018

Uh oh!

kennytm commented Mar 28, 2018

Uh oh!

bors commented Mar 29, 2018

Uh oh!

bors commented Mar 29, 2018

Uh oh!

kivikakk commented Mar 31, 2018

Uh oh!

petrochenkov commented Mar 31, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kivikakk commented Mar 31, 2018

Uh oh!

Amanieu commented Apr 3, 2018

Uh oh!

Amanieu commented Apr 3, 2018

Uh oh!

tejom commented Apr 3, 2018

Uh oh!

Amanieu commented Apr 3, 2018

Uh oh!

tejom commented Apr 4, 2018

Uh oh!

Amanieu commented Apr 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

tejom commented Mar 23, 2018•
edited
Loading

petrochenkov commented Mar 31, 2018•
edited
Loading