Lints on AST, remove ExprParen #28349

nrc · 2015-09-11T08:18:57Z

Manishearth · 2015-09-11T12:43:00Z

I'd personally prefer if there were different kinds of lints entirely (not one kind of lint with different available contexts -- this means the lint will be run multiple times). So, basically, the LintStore would contain both EarlyLintPasses and LateLintPasses. It also keeps the lint traits easier to understand. If we want to have "MIR lints" in the future, the design is easily extended.

But I'm not insistent on this, the current model is fine too.

Aside from that and the check_mac thing, LGTM

Manishearth · 2015-09-11T12:43:47Z

src/librustc/lint/mod.rs

+    fn check_explicit_self(&mut self, _: &LateContext, _: &hir::ExplicitSelf) { }
+    // Note that you shouldn't implement both check_mac and check_ast_mac,
+    // because then your lint will be called twice. Prefer check_ast_mac.
+    fn check_mac(&mut self, _: &LateContext, _: &ast::Mac) { }


Why would we have a macro in the HIR tree? Isn't HIR post-expansion?

(Actually, so is the AST at the time of early lint checking, do we need a check_mac of any form here?)

@Manishearth

Why would we have a macro in the HIR tree? Isn't HIR post-expansion?

I believe these are exported macro definitions, not macro uses.

@nikomatsakis http://manishearth.github.io/rust-internals-docs/syntax/ast/enum.Mac_.html

Docs say otherwise

nrc · 2015-09-11T19:08:59Z

My first idea for this was having two kinds of lints (early and late), but I decided against basically for backwards compatibility and simplicity. With the changes as they stand, lint authors only need to import a trait. I thought having to use different pass names, etc. would be too much of a breaking change. However, I guess if we are going to break lints even a little, then breaking them a lot is not much worse. I also thought it was easier having one big lint, rather than lots of little ones, but I'm not really sure about that.

If the backcompat issue is not too big a deal, then I can redo this with different lint passes pretty easily.

Manishearth · 2015-09-11T19:47:21Z

I don't think it would be a major backcompat issue; given that it's just one trait that can be sed'ed (most lints work at the HIR stage). Only AST lints will have some backcompat issues (which is a tiny bit more sed)

petrochenkov · 2015-09-13T17:09:44Z

I'm waiting for this to land and then will try to finish #6993 by replacing Idents with Names in HIR and below.

nikomatsakis · 2015-09-14T17:42:28Z

I defer to @Manishearth (aka Mr Lint) when it comes to the backwards compat pain. Basically seems like we ought to redo this PR as described?

Manishearth · 2015-09-14T23:12:41Z

I'm okay with the status quo, but I'd prefer the multitrait approach, yeah. I don't think it would be a major backcompat issue. Most lints out there are conceptually HIR lints, and we'd just need to replace some trait imports.

The original HIR changes (which had a lot of fiddly, non-sed-able import changes) were larger backcompat issues than this could be.

nrc · 2015-09-15T01:58:55Z

Now with multiple traits

bors · 2015-09-15T03:25:27Z

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

Manishearth · 2015-09-15T03:35:02Z

LGTM

bors · 2015-09-16T02:22:24Z

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

nrc · 2015-09-16T02:51:19Z

@bors: r=manishearth

bors · 2015-09-16T02:51:19Z

📌 Commit 664b874 has been approved by manishearth

bors · 2015-09-16T03:59:58Z

⌛ Testing commit 664b874 with merge 71bdae1...

bors · 2015-09-16T04:07:06Z

💔 Test failed - auto-mac-32-opt

Manishearth · 2015-09-16T04:09:13Z

../src/librustc/middle/expr_use_visitor.rs:283:22: 287:10 error: cannot infer an appropriate lifetime for lifetime parameter `'tcx` due to conflicting requirements
../src/librustc/middle/expr_use_visitor.rs:283         let result = ExprUseVisitor {
../src/librustc/middle/expr_use_visitor.rs:284             typer: typer,
../src/librustc/middle/expr_use_visitor.rs:285             mc: mc::MemCategorizationContext::new(typer),
../src/librustc/middle/expr_use_visitor.rs:286             delegate: delegate,
../src/librustc/middle/expr_use_visitor.rs:287         };
../src/librustc/middle/expr_use_visitor.rs:279:5: 290:6 help: consider using an explicit lifetime parameter as shown: fn new(delegate: &'d mut Delegate<'tcx>, typer: &'t infer::InferCtxt<'a, 't>)
 -> ExprUseVisitor<'d, 't, 'a, 't>
../src/librustc/middle/expr_use_visitor.rs:279     pub fn new(delegate: &'d mut Delegate<'tcx>,
../src/librustc/middle/expr_use_visitor.rs:280                typer: &'t infer::InferCtxt<'a, 'tcx>)
../src/librustc/middle/expr_use_visitor.rs:281                -> ExprUseVisitor<'d,'t,'a,'tcx>
../src/librustc/middle/expr_use_visitor.rs:282     {
../src/librustc/middle/expr_use_visitor.rs:283         let result = ExprUseVisitor {
../src/librustc/middle/expr_use_visitor.rs:284             typer: typer,
                                               ...
error: aborting due to previous error

bors · 2015-09-16T10:36:46Z

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

There is a minor [breaking-change] for lint authors - some functions which were previously defined on `lint::Context` have moved to a trait - `LintContext`, you may need to import that trait to avoid name resolution errors.

[breaking-change] for lint authors You must now implement LateLintPass or EarlyLintPass as well as LintPass and use either register_late_lint_pass or register_early_lint_pass, rather than register_lint_pass.

nrc · 2015-09-17T01:53:37Z

@bors: r=manishearth

bors · 2015-09-17T01:53:38Z

📌 Commit ed61a64 has been approved by manishearth

bors · 2015-09-17T03:05:57Z

⌛ Testing commit ed61a64 with merge d16129b...

@nikomatsakis

r? @nikomatsakis, f? @Manishearth

bors · 2015-09-17T04:48:09Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned nikomatsakis Sep 11, 2015

Manishearth reviewed Sep 11, 2015
View reviewed changes

nrc force-pushed the ast-lints branch from 9f187ed to d9e2b81 Compare September 15, 2015 01:58

nrc force-pushed the ast-lints branch from d9e2b81 to 60c9210 Compare September 15, 2015 05:32

nrc force-pushed the ast-lints branch from 60c9210 to 664b874 Compare September 16, 2015 02:51

nrc added 6 commits September 17, 2015 12:16

Add an early lint pass for lints that operate on the AST

76856e1

There is a minor [breaking-change] for lint authors - some functions which were previously defined on `lint::Context` have moved to a trait - `LintContext`, you may need to import that trait to avoid name resolution errors.

Change the unused parens lint to operate on the AST

70c6fb9

Remove hir::ExprParen

d21bfff

Changes to tests

c1084a3

Change to a multi-trait approach

a642d85

[breaking-change] for lint authors You must now implement LateLintPass or EarlyLintPass as well as LintPass and use either register_late_lint_pass or register_early_lint_pass, rather than register_lint_pass.

rebasing

ed61a64

nrc force-pushed the ast-lints branch from 664b874 to ed61a64 Compare September 17, 2015 01:53

bors added a commit that referenced this pull request Sep 17, 2015

Auto merge of #28349 - nrc:ast-lints, r=manishearth

d16129b

r? @nikomatsakis, f? @Manishearth

bors merged commit ed61a64 into rust-lang:master Sep 17, 2015

This was referenced Sep 17, 2015

Split up lints #28429

Merged

Correctly walk import lists in AST visitors #28364

Merged

llogiq mentioned this pull request Sep 22, 2015

TWiR Release 97 (21 September 2015). rust-lang/this-week-in-rust#125

Merged

nrc mentioned this pull request Sep 24, 2015

HIR: separate AST and HIR lints #28145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Lints on AST, remove ExprParen #28349

Lints on AST, remove ExprParen #28349

nrc commented Sep 11, 2015

Manishearth commented Sep 11, 2015

Manishearth Sep 11, 2015

nikomatsakis Sep 11, 2015

Manishearth Sep 11, 2015

nrc commented Sep 11, 2015

Manishearth commented Sep 11, 2015

petrochenkov commented Sep 13, 2015

nikomatsakis commented Sep 14, 2015

Manishearth commented Sep 14, 2015

nrc commented Sep 15, 2015

bors commented Sep 15, 2015

Manishearth commented Sep 15, 2015

bors commented Sep 16, 2015

nrc commented Sep 16, 2015

bors commented Sep 16, 2015

bors commented Sep 16, 2015

bors commented Sep 16, 2015

Manishearth commented Sep 16, 2015

bors commented Sep 16, 2015

nrc commented Sep 17, 2015

bors commented Sep 17, 2015

bors commented Sep 17, 2015

bors commented Sep 17, 2015

Lints on AST, remove ExprParen #28349

Lints on AST, remove ExprParen #28349

Conversation

nrc commented Sep 11, 2015

Manishearth commented Sep 11, 2015

Manishearth Sep 11, 2015

Choose a reason for hiding this comment

nikomatsakis Sep 11, 2015

Choose a reason for hiding this comment

Manishearth Sep 11, 2015

Choose a reason for hiding this comment

nrc commented Sep 11, 2015

Manishearth commented Sep 11, 2015

petrochenkov commented Sep 13, 2015

nikomatsakis commented Sep 14, 2015

Manishearth commented Sep 14, 2015

nrc commented Sep 15, 2015

bors commented Sep 15, 2015

Manishearth commented Sep 15, 2015

bors commented Sep 16, 2015

nrc commented Sep 16, 2015

bors commented Sep 16, 2015

bors commented Sep 16, 2015

bors commented Sep 16, 2015

Manishearth commented Sep 16, 2015

bors commented Sep 16, 2015

nrc commented Sep 17, 2015

bors commented Sep 17, 2015

bors commented Sep 17, 2015

bors commented Sep 17, 2015