Set CompilerDesugaring expn_info for all desugared expressions #39766

sinkuu · 2017-02-12T16:48:50Z

This PR adds CompilerDesugaring expn_info for all compiler desugared expressions. This will ease writing lints which need to exclude them (for example https://github.com/Manishearth/rust-clippy/pull/1513).

However, it will add in this macro invocation note for every error on syntax-sugar span, which is unhelpful. The 2nd commit removes them.

sinkuu · 2017-02-15T02:59:15Z

It looks like the highfive bot has missed this PR. Can anyone review this?

bors · 2017-02-25T15:03:36Z

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

oli-obk · 2017-02-27T11:37:50Z

r? @Manishearth

noone seems to want to take this, and it's clippy related ;)

alexcrichton · 2017-02-27T20:43:21Z

cc @rust-lang/compiler

eddyb · 2017-02-27T20:44:11Z

r? @jseyfried

Manishearth · 2017-02-27T20:44:51Z

I would avoid giving sole r+ on PRs that affect clippy

nrc · 2017-02-28T21:13:33Z

I think we should not do this - we have avoided it in the past. I have tried to keep a strong distinction in the compiler around generated code - in particular lowered code is not generated code. The contract is basically that if the user should recognise that the code is generated (i.e., they used a macro or other form of codegen) then the span should have an expn_info and error messages should be explicit about the expansion. Tools can recognise that the source code does not reflect the compiler's representation and can 'correct' that via the expansion stack.

OTOH, code that the user cannot recognise as generated (e.g., for loops) should not have an expn_info and error messages should be opaque about the lowering - i.e., they must behave as if the source code were first class. Spans should be adjusted by lowering in such a way that tools should be able to cope with just the spans and not need 'expansion' information.

I'm not sure how best to help Clippy here. I suspect it should be using a higher level API to the compiler's info, but not sure how that should work in general.

eddyb · 2017-02-28T21:21:04Z

@nrc For precise spans we need to do this though, however I agree users shouldn't be able to see it.

nrc · 2017-02-28T21:43:22Z

@eddyb yeah, long term I think we should consider changes - I do envision lowering being reflected in the spans, but distinct from other kinds of generated code (this might just be a flag, but ideally we'd have some way of handling the 'macro definition').

eddyb · 2017-02-28T21:53:10Z

@nrc FWIW we already create such spans - we have to, for allow_internal_unstable in some cases.

nrc · 2017-02-28T22:21:40Z

@eddyb I'm not sure what you mean by "such spans" - if we did this for lowered expressions then this PR would be redundant, no? I'm not sure how the stability checking works though.

eddyb · 2017-02-28T22:31:28Z

@nrc We only do it when have to for stability, look for calls to the allow_internal_unstable method.

nikomatsakis · 2017-03-02T18:11:34Z

@nrc @eddyb

Would it be helpful to have a "lowered code" distinction that applies specifically to lowering (and not things the user understands as generated)? That's what I would expect. (Caveat: I've not looked at PR yet.)

nikomatsakis · 2017-03-02T18:12:15Z

src/test/compile-fail/if-let-arm-types.rs

@@ -13,6 +13,8 @@ fn main() {
        //~^ expected (), found integral variable
        //~| expected type `()`
        //~| found type `{integer}`
+        //~| NOTE: in this expansion of desugaring of `if let`
+        //~| NOTE: in this expansion of desugaring of `if let`


I definitely think we should not be writing this to the output.

These lines are required by compiletest, but not shown in rustc output (see the second commit).

nikomatsakis · 2017-03-02T18:12:24Z

src/test/compile-fail/E0297.rs

@@ -14,4 +14,5 @@ fn main() {
    for Some(x) in xs {}
    //~^ ERROR E0297
    //~| NOTE pattern `None` not covered
+    //~| NOTE in this expansion of desugaring of `for`


eddyb · 2017-03-02T18:18:07Z

@nikomatsakis We already have that distinction, we use it for creating allow_internal_unstable spans during HIR lowering.

nikomatsakis · 2017-03-02T18:31:56Z

@eddyb so ... what are we arguing about exactly? I must be confused.

eddyb · 2017-03-02T18:37:21Z

@nikomatsakis I am saying we're already using this in some cases, and I'm pretty sure we agree we need unique spans in the future, so I'm in favor of making this change with no user-visible output.

nikomatsakis · 2017-03-02T18:39:05Z

@eddyb ok I agree with you :)

nrc · 2017-03-02T20:24:05Z

So I assume the allow_internal_unstable change had a negative impact on save-analysis and that doing this universally will break it further. It took a fair amount of tweaking to get the HIR lowering working with save-analysis and that change goes against that design. I would like to see save-analysis fixed before going further down this road (and it would be nice, in general, to get pinged about any changes to spans or macro expansion, rather than have them buried in a 4000 line PR and finding out 4 months later).

eddyb · 2017-03-02T20:28:48Z

I'm not sure how that makes sense? save-analysis doesn't look at HIR spans, does it?

nrc · 2017-03-02T21:45:33Z

save-analysis certainly looks at HIR nodes (we have to in order to get type check info). We also look at spans from deep inside nodes. I'm not sure if we actually look at spans from HIR nodes because of that, so you are right that it is probably not as bad as I thought. However, it is certainly possible that we do look at some HIR span somewhere, so if we move to doing the span stuff everywhere, then I'd like to make sure we don't break any save-analysis stuff.

eddyb · 2017-03-02T21:50:01Z

@nrc You should auto-generate a corpus of tests (from run-pass tests or something), IMO.
That way we get to know ahead of time if we're breaking save-analysis.

nrc · 2017-03-07T03:52:04Z

@eddyb yeah, tests for save-analysis have been on my mind for like the last 6 months. Someday I'll implement them

bors · 2017-03-14T13:23:34Z

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

oli-obk · 2017-03-14T14:07:01Z

So what action is required here?

nrc · 2017-03-15T19:57:19Z

@oli-obk @sinkuu I think this PR needs changing so that the changes are not user-visible (see @nikomatsakis's comments). There is apparently a distinction that can be made between lowered and generated code and that should be used (I was under the impression we couldn't directly do this, but perhaps I don't understand exactly what is meant here) - the stuff in this PR should use that.

Finally, we should establish that this does not break save-analysis. To do that, fine the two run-make save-analysis tests. Each should emit a JSON or CSV file when run. Compare the files before and after applying this PR, they should be identical. If the post-PR file is smaller, then there are issues that need addressing.

oli-obk · 2017-03-24T08:10:45Z

@sinkuu needs another rebase

@nrc: the issues have been addressed. The change is only user-visible in the json output.

nrc · 2017-03-27T02:11:22Z

@sinkuu did you get a chance to compare the save-analysis output?

nrc · 2017-03-27T02:14:31Z

The change is only user-visible in the json output.

I think desugarings should not be user-visible in JSON output either - the expectation is that JSON errors are shown to users fairly directly by tools. I wouldn't expect an IDE (for example) to show the user the macro expansion of a desugared language feature.

carols10cents · 2017-04-14T17:46:46Z

☔️ merge conflicts!

arielb1 · 2017-04-18T08:56:45Z

@sinkuu & @nrc - what is the status of this PR?

arielb1 · 2017-04-25T09:33:40Z

🕸 We haven't seen activity on this PR for 2 weeks 🕸, so we're closing it to reduce our backlog. Feel free to reopen if you still want to work on it.

sinkuu force-pushed the desugaring_span branch from 0e75e7b to b6d8569 Compare February 26, 2017 03:36

rust-highfive assigned jseyfried Feb 27, 2017

nikomatsakis reviewed Mar 2, 2017

View reviewed changes

Use CompilerDesugaring span for desugared expressions

efcef24

sinkuu added 2 commits March 21, 2017 15:59

Do not show in this macro invocation for compiler desugared spans

c4948e7

Add run-make test

2d17aff

sinkuu force-pushed the desugaring_span branch from b6d8569 to 2d17aff Compare March 21, 2017 08:44

Mark-Simulacrum added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 14, 2017

arielb1 closed this Apr 25, 2017

This was referenced Jun 20, 2017

Improve ? error messages per RFC 1859 #35946

Closed

False positive in used_underscore_binding on _next in a for-loop rust-lang/rust-clippy#1846

Closed

llogiq mentioned this pull request Jun 25, 2017

change binding name of for loop lowering to appease clippy #42871

Merged

oli-obk mentioned this pull request Jun 28, 2017

Impl Try for Option #42526

Merged

sinkuu deleted the desugaring_span branch July 8, 2019 12:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Set CompilerDesugaring expn_info for all desugared expressions #39766

Set CompilerDesugaring expn_info for all desugared expressions #39766

sinkuu commented Feb 12, 2017 •

edited

Loading

sinkuu commented Feb 15, 2017

bors commented Feb 25, 2017

oli-obk commented Feb 27, 2017 •

edited

Loading

alexcrichton commented Feb 27, 2017

eddyb commented Feb 27, 2017

Manishearth commented Feb 27, 2017

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017 •

edited

Loading

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017

nikomatsakis commented Mar 2, 2017

nikomatsakis Mar 2, 2017

sinkuu Mar 21, 2017

nikomatsakis Mar 2, 2017

eddyb commented Mar 2, 2017

nikomatsakis commented Mar 2, 2017

eddyb commented Mar 2, 2017

nikomatsakis commented Mar 2, 2017

nrc commented Mar 2, 2017

eddyb commented Mar 2, 2017

nrc commented Mar 2, 2017

eddyb commented Mar 2, 2017

nrc commented Mar 7, 2017

bors commented Mar 14, 2017

oli-obk commented Mar 14, 2017

nrc commented Mar 15, 2017

oli-obk commented Mar 24, 2017

nrc commented Mar 27, 2017

nrc commented Mar 27, 2017

carols10cents commented Apr 14, 2017

arielb1 commented Apr 18, 2017 •

edited

Loading

arielb1 commented Apr 25, 2017

Set CompilerDesugaring expn_info for all desugared expressions #39766

Set CompilerDesugaring expn_info for all desugared expressions #39766

Conversation

sinkuu commented Feb 12, 2017 • edited Loading

sinkuu commented Feb 15, 2017

bors commented Feb 25, 2017

oli-obk commented Feb 27, 2017 • edited Loading

alexcrichton commented Feb 27, 2017

eddyb commented Feb 27, 2017

Manishearth commented Feb 27, 2017

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017 • edited Loading

nrc commented Feb 28, 2017

eddyb commented Feb 28, 2017

nikomatsakis commented Mar 2, 2017

nikomatsakis Mar 2, 2017

Choose a reason for hiding this comment

sinkuu Mar 21, 2017

Choose a reason for hiding this comment

nikomatsakis Mar 2, 2017

Choose a reason for hiding this comment

eddyb commented Mar 2, 2017

nikomatsakis commented Mar 2, 2017

eddyb commented Mar 2, 2017

nikomatsakis commented Mar 2, 2017

nrc commented Mar 2, 2017

eddyb commented Mar 2, 2017

nrc commented Mar 2, 2017

eddyb commented Mar 2, 2017

nrc commented Mar 7, 2017

bors commented Mar 14, 2017

oli-obk commented Mar 14, 2017

nrc commented Mar 15, 2017

oli-obk commented Mar 24, 2017

nrc commented Mar 27, 2017

nrc commented Mar 27, 2017

carols10cents commented Apr 14, 2017

arielb1 commented Apr 18, 2017 • edited Loading

arielb1 commented Apr 25, 2017

sinkuu commented Feb 12, 2017 •

edited

Loading

oli-obk commented Feb 27, 2017 •

edited

Loading

eddyb commented Feb 28, 2017 •

edited

Loading

arielb1 commented Apr 18, 2017 •

edited

Loading