Re-implement lint with less emphasis on item ids by alexcrichton · Pull Request #6093 · rust-lang/rust

alexcrichton · 2013-04-28T03:12:12Z

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

alexcrichton · 2013-04-28T03:12:12Z

Closes#2647

This way it's much easier to add lints throughout compilation correctly, and
functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments).

As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

pcwalton · 2013-04-28T05:09:41Z

nit: extraneous parentheses.

alexcrichton · 2013-04-28T14:59:37Z

Updated with less parentheses and allocations

alexcrichton · 2013-04-28T15:44:32Z

Hmm... I also just found that some imports are getting warned about when they probably shouldn't be, so I'll update once I've investigated that.

alexcrichton · 2013-04-28T22:01:50Z

Should be good to go now, there was an import in quote_expr! which was generated so I just marked it as public so it wouldn't be warned about.

alexcrichton · 2013-05-02T17:52:50Z

@pcwalton r?

graydon · 2013-05-02T20:51:01Z

I'm a little nervous about this. The existing system had to be revised several times before it was considered "correct". I .. actually don't remember what algorithm was settled on, and I feel like maybe we don't have enough testing in place to know if this perturbs the logic.

graydon · 2013-05-02T20:55:51Z

Sorry, I guess that was a bit useless as feedback. Here are my concerns:

I think we currently do something like "assign lint levels early in the program, then check them later". I don't actually know if that's the order we do things in, but I also can't quite tell if this patch preserves that structure. (Niko wants to change it, but that's a different matter)
I am not sure spans are ... entirely robust, in terms of being unique and reliable identifiers-of-code. We generate artificial ones in some contexts and there is a fair amount of subtlety concerning spans originating in macros to consider. I'm not sure if this change is safe in that sense.

I'm of course glad to see attention on the lint system! Maybe I am just being my usual paranoid self and these concerns are not so serious? Happy to entertain some discussion on the matter. I just see this sitting here and I'm not r+'ing it due to a little hesitation.

nikomatsakis · 2013-05-02T20:56:36Z

I agree lint is kind of not working out, but I'm not sure this is the solution. I had the thought of doing an inverse style:

lint would be the only to report lintable errors, but it would not do any error detection itself
earlier passes would find lintable errors and add them into tables that would be read by lint
(perhaps even just a single master table that maps from node_id -> ~[LintableError] or something)
- this seems like it addresses the most annoying part of the refactoring required, which is that you currently must know the item id as well as the id where the error occurs etc. There will still be some work for those parts of the code that only pass spans around, but it's probably better to work more on ids anyhow

alexcrichton · 2013-05-02T22:05:49Z

Thanks for the comments! I wanted to do what @nikomatsakis was thinking, but I couldn't figure out a reliable way of always checking every node_id, although that approach is definitely a lot more robust than using spans.

Is there a way to iterate over every node id in the ast, or would I need to add something like visit_node_id to visit.rs? That was the main trouble I ran into when figuring what the minimum amount of reliable information that various passes like liveness could pass into lint for later warning.

nikomatsakis · 2013-05-02T23:51:23Z

ast_util offers a visit_ids function actually. We have a number of places that rely on being able to enumerate the ids (inling, for example) so if that's broken, there will be similar bugs elsewhere.

alexcrichton · 2013-05-03T06:23:10Z

Just updated to no longer use spans, but rather use node_ids to attribute lints to nodes. Throughout compilation, any pass can invoke add_lint with a node id, span, and message. Then when time for lint comes around, it'll walk the ast looking at nodes, and if the node is in the hash table of lints added, the lint is processed with the current lint settings.

alexcrichton · 2013-05-03T19:22:44Z

I just updated with a fix to the tests, but I also realized that there's an unfortunate side effect of this in that linting runs 3x slower (at least on the core crate). There's a super-easy fix which is to not create a new visitor every time you run a new lint check, but when I implemented that it was complaining about leaked memory on exit...

nikomatsakis · 2013-05-04T00:12:39Z

Hmm. That is too bad, I suspect that may be a side-effect of how visit_id is written, which I think is pretty inefficienct. How much time are we talking about? It might be worth landing and then trying to fixup visit_id to run more efficiently.

alexcrichton · 2013-05-04T00:14:49Z

libcore took ~0.16s in linting before this change, and it takes about 0.46s after the change. With my change that leaks memory (but only allocates all the visitors once), it takes ~0.06s

nikomatsakis · 2013-05-04T17:59:27Z

I see. It seems to me that this timing regression is small in the
scheme of things. In the interests of not introducing artificial
dependencies, I'd prefer to land this change and then open an issue
and a second PR with your performance optimization that causes a
leak. Either you or one of us can then working on debugging the source
of that leak. @graydon, @pcwalton, you two also commented on this bug,
does that sound like a reasonable plan?

graydon · 2013-05-07T03:19:02Z

That sounds ok to me. Getting the structure of lint right is pretty important.

I'm curious about the change that leaks memory! Please do file a followup with as much detail as possible. Obviously that shouldn't be happening 😦

graydon · 2013-05-07T03:19:14Z

(also, seems this now needs a rebase)

alexcrichton · 2013-05-07T04:08:08Z

Just rebased, and the errors may have been fixed by the huge borrowck fix that just landed. Once this goes through I'll look into optimizing it and doing a more thorough inspection.

alexcrichton · 2013-05-07T06:08:27Z

Apparently my suspicions were correct, or I didn't know what I was doing before. Regardless I just added a commit with the optimizations I had in mind, and it does indeed run a large amount faster now (just the lint pass, obviously).

alexcrichton · 2013-05-14T03:53:53Z

r? @graydon

sanxiyn · 2013-05-15T15:33:15Z

This needs rebase again. :(

This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Achieves ~3x speedup on lint passes for libcore

Closes#2647 This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted. This involved pretty much rewriting how lints are emitted. Beforehand, only items could alter the lint settings, so whenever a lint was added it had to be associated with whatever item id it was coming from. I removed this (possibly questionably) in favor of just specifying a span and a message when adding a lint. When lint checking comes around, it looks at all the lints and sees which node with attributes best encloses it and uses that level of linting. This means that all consumer code doesn't have to deal with what item things came from (especially because functions on impls aren't items). More details of this can be found in the code (and comments). As a bonus, I managed to greatly simplify emission of lints in resolve.rs about unused imports. Now instead of it manually tracking what the lint level is, it's all moved over into the lint module (as is to be expected).

…hearth needless arbitrary self: handle macros This fixes two cases related to macros: * If the parameter comes from expansion, do not lint as the user has no possibility of changing it. This is not directly related to the fixed issue, but we should probably do that. * If *only* the lifetime name comes from expansion, lint, but allow the user decide the name of the lifetime. In the related issue, the lifetime was unnamed and then renamed by `async_trait`, so just removing the name in the suggestion would work, but in the general case a macro can rename a lifetime that was named differently, and we can't reliably know that name anymore. As a hint for the reviewer, the expanded code for the test can be checked with this command (from the root dir of the repo): ```sh rustc -L target/debug/test_build_base/needless_arbitrary_self_type_unfixable.stage-id.aux -Zunpretty=expanded tests/ui/needless_arbitrary_self_type_unfixable.rs ``` changelog: [`needless_arbitrary_self_type`]: handle macros Fixesrust-lang#6089

6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

pcwalton reviewed Apr 28, 2013
View reviewed changes

Comment threadsrc/librustc/middle/lint.rs Outdated
pcwaltonApr 28, 2013
Copy link
Copy Markdown
Contributor
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: extraneous parentheses.

This was referenced May 9, 2013
Show lint names #6325
Closed
"Unused imports" warning has false-positives in the presence of syntax extensions #6371
Closed

alexcrichton closed this May 11, 2013

alexcrichton reopened this May 11, 2013

alexcrichton added 2 commits May 17, 2013 00:49

Re-implement lint with less emphasis on item ids
030c666
This way it's much easier to add lints throughout compilation correctly, and functions on impls can alter the way lints are emitted.

Optimize lint passes to perform far fewer allocations
1daaf78
Achieves ~3x speedup on lint passes for libcore

Change lints to indicate exactly where the level was set
4d44abd

bors closed this May 17, 2013

alexcrichton mentioned this pull request May 17, 2013
Warning directives aren't respected when in front of methods in impls #2647
Closed

U007D pushed a commit to U007D/rust-mos that referenced this pull request Aug 21, 2026
Mergerust-lang#6093
4ecab09
6093: Add panic_context module for better panic messages r=matklad a=matklad Co-authored-by: Aleksey Kladov <aleksey.kladov@gmail.com>

Uh oh!

Uh oh!

Conversation

alexcrichton commented Apr 28, 2013

Uh oh!

pcwaltonApr 28, 2013

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented May 2, 2013

Uh oh!

graydon commented May 2, 2013

Uh oh!

graydon commented May 2, 2013

Uh oh!

nikomatsakis commented May 2, 2013

Uh oh!

alexcrichton commented May 2, 2013

Uh oh!

nikomatsakis commented May 2, 2013

Uh oh!

alexcrichton commented May 3, 2013

Uh oh!

alexcrichton commented May 3, 2013

Uh oh!

nikomatsakis commented May 4, 2013

Uh oh!

alexcrichton commented May 4, 2013

Uh oh!

nikomatsakis commented May 4, 2013

Uh oh!

graydon commented May 7, 2013

Uh oh!

graydon commented May 7, 2013

Uh oh!

alexcrichton commented May 7, 2013

Uh oh!

alexcrichton commented May 7, 2013

Uh oh!

alexcrichton commented May 14, 2013

Uh oh!

sanxiyn commented May 15, 2013

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Uh oh!

Uh oh!

Conversation

alexcrichton commented Apr 28, 2013

Uh oh!

pcwaltonApr 28, 2013

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented Apr 28, 2013

Uh oh!

alexcrichton commented May 2, 2013

Uh oh!

graydon commented May 2, 2013

Uh oh!

graydon commented May 2, 2013

Uh oh!

nikomatsakis commented May 2, 2013

Uh oh!

alexcrichton commented May 2, 2013

Uh oh!