New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve parsing diagnostic for negative supertrait bounds #57364

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@hdhoang
Copy link
Contributor

hdhoang commented Jan 6, 2019

closes #33418

r? @estebank

Show resolved Hide resolved src/libsyntax/ast.rs Outdated
BytePos(sp.lo().0 - 1 - spaces.len() as u32));
err.span_suggestion_with_applicability(span_to_remove,
"remove this trait bound",
"".into(), Applicability::MachineApplicable);

This comment has been minimized.

@hdhoang

hdhoang Jan 6, 2019

Contributor

This is still too crude to be machine-applicable:

trait Tr3: !Sync + Copy {}

will be left as broken Tr3 + Copy {}. Does anyone have a suggestion? I will see if moving this back to the parser provides more context to fix such cases properly.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 6, 2019

The job x86_64-gnu-llvm-6.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0ec3f578:start=1546782125819050653,finish=1546782196239804965,duration=70420754312
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#pull-requests-and-security-restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-6.0
---
[01:00:27] .................................................................................................... 3500/5299
[01:00:31] .................................................................................................... 3600/5299
[01:00:34] ......................................ii............................................................ 3700/5299
[01:00:36] .........................................................i.......................................... 3800/5299
[01:00:37] .........................................F.......................................................... 3900/5299
[01:00:42] .................................................................................................... 4100/5299
[01:00:52] .................................................................................................... 4200/5299
[01:00:55] .................................................................................................... 4300/5299
[01:00:59] .................................................................................................... 4400/5299
[01:00:59] .................................................................................................... 4400/5299
[01:01:02] ........................................................i........................................... 4500/5299
[01:01:08] .................................................................................................... 4600/5299
[01:01:12] .................................................................................................... 4700/5299
[01:01:15] .................................................................................................... 4800/5299
[01:01:19] .................................................................................................... 4900/5299
[01:01:22] .................................................................................................... 5000/5299
[01:01:31] - LL | trait Tr4: !Sized + !Send + Copy + !Sync + !Unpin {} //~ ERROR negative trait bounds are not supported
[01:01:31] -    |                                  --^^^^^
[01:01:31] + LL | trait Tr4: !Sized + Copy + !Sync + !Unpin {} //~ ERROR negative trait bounds are not supported
[01:01:31] +    |                                  --^^^^^^
[01:01:31] 47    |                                  help: remove this trait bound
[01:01:31] 48 
[01:01:31] 
[01:01:31] 49 error: negative trait bounds are not supported
[01:01:31] 49 error: negative trait bounds are not supported
[01:01:31] -   --> $DIR/issue-33418.rs:6:44
[01:01:31] -    |
[01:01:31] - LL | trait Tr4: !Sized + !Send + Copy + !Sync + !Unpin {} //~ ERROR negative trait bounds are not supported
[01:01:31] -    |                                          --^^^^^^
[01:01:31] -    |                                          help: remove this trait bound
[01:01:31] - 
[01:01:31] - error: negative trait bounds are not supported
[01:01:31] 58   --> $DIR/issue-33418.rs:7:12
[01:01:31] 58   --> $DIR/issue-33418.rs:7:12
[01:01:31] 59    |
[01:01:31] 60 LL | trait Tr5: !Sized + !Send {} //~ ERROR negative trait bounds are not supported
[01:01:31] 70    |                   |
[01:01:31] 71    |                   help: remove this trait bound
[01:01:31] 72 
[01:01:31] - error: aborting due to 9 previous errors
---
[01:01:31] 
[01:01:31] thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:495:22
[01:01:31] 
[01:01:31] 
[01:01:31] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/travis_time:end:2a9fb0c4:start=1546782205172551672,finish=1546785897059313420,duration=3691886761748
travis_time:start:03060326
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sun Jan  6 14:44:57 UTC 2019
Sun, 06 Jan 2019 14:44:57 GMT

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@estebank
Copy link
Contributor

estebank left a comment

It seems like the tests are currently broken. Added an alternative way of getting the suggestion span.

@@ -5107,7 +5109,32 @@ impl<'a> Parser<'a> {
if has_parens {
self.expect(&token::CloseDelim(token::Paren))?;
}
let poly_trait = PolyTraitRef::new(lifetime_defs, path, lo.to(self.prev_span));
let poly_span = lo.to(self.prev_span);

This comment has been minimized.

@estebank

estebank Jan 7, 2019

Contributor

Looking at the travis output it seems that you need to fix up the suggestion span:

20	LL | trait Tr3: !Sync + Unpin {} //~ ERROR negative trait bounds are not supported
-	   |            ^^^^^--
-	   |            |
-	   |            help: remove this trait bound
+	   |          --^^^^^
+	   |          |
+	   |          help: remove this trait bound

This comment has been minimized.

@hdhoang

hdhoang Jan 7, 2019

Contributor

my bad, I was leaving code and test quite out of sync. With this span, I was trying to leave the fixed code syntactically valid, thus I tried to remove the following + instead of the preceding :. In the general case, I imagine this will require much more info passed into this function.

This comment has been minimized.

@hdhoang

hdhoang Jan 14, 2019

Contributor

Thanks, I have a WIP solution below.

- 1 // for the `+` or `:`
- spaces.len() as u32))
}
}

This comment has been minimized.

@estebank

estebank Jan 7, 2019

Contributor

Instead of looking back in the source_map, wouldn't it make more sense to modify parse_ty_param to pass in the : span to parse_generic_bounds_common (so that you can remove it if necessary) and also keep track of the prior + later in this method (!self.eat_plus())? That way you will use either one of those two spans to create the suggestion span. Something along the lines of:

last_plus_sp = None;
if self.eat_plus() {
    last_plus_sp = Some(self.last_span);
}

/* ... */

let span_to_remove = last_plus_sp.unwrap_or(colon_sp).until(poly_span);

This comment has been minimized.

@hdhoang

hdhoang Jan 7, 2019

Contributor

thank you for your mentoring! I was quite ready to give up on rustfixing this. I'll need some time to grok and implement your method, and making more edge cases.

Since this is the parser, iteration time degraded big time compared to modifying the AST validation pass. Do you have any tip for reducing edit->test time, e.g. incremental in config.toml? I promise not to waste the project's CI budget like that.

This comment has been minimized.

@estebank

estebank Jan 14, 2019

Contributor

Sadly, hacking on parser.rs is just inherently slow. I run ./x.py test src/test/ui --stage1 --bless, and that takes a while. Learning to use the profiler can be worth it, but for the most part I just switch to working on something else/fetch tea while rustc does its thing :-|

@stokhos

This comment has been minimized.

Copy link

stokhos commented Jan 14, 2019

Ping from triage @hdhoang Have you been able to make any progress on this?

@hdhoang

This comment has been minimized.

Copy link
Contributor

hdhoang commented Jan 14, 2019

thank you, I have a rough outline of the necessary flow inside the loop. I will describe most of it later while working it out locally.

@hdhoang hdhoang force-pushed the hdhoang:33418_negative_bounds branch from 710db3b to 3ad7e5d Jan 14, 2019

@hdhoang

This comment has been minimized.

Copy link
Contributor

hdhoang commented Jan 14, 2019

I have pushed a WIP flow for review.

Firstly, I have renamed the supertraits in tests to make talking about them easier.

On to the parser, to pass the colon span into the loop, I have added a colon_span argument to parse_generic_bounds/parse_generic_bounds_common, then passing DUMMY_SP at irrelevant callsites. The name isn't descriptive, but hopefully using a dummy span elsewhere makes it clear.

Next, I have implemented your last_plus_span flow, and created a flag to track whether we need to promote a plus to a colon to keep the code valid. I temporarily used struct_span_err here, but I think a warning/note/help message hanging on to last_plus_span is more appropriate. I will find a more suitable macro for that, and a clearer message for "will become the first one".

@estebank
Copy link
Contributor

estebank left a comment

The output looks superb.

I think that we can iterate on the changes to parse_generic_bounds_common, as you're using a DUMMY_SP in some cases. I think that replacing that with None would be safer, and just avoid giving any suggestion if colon_span isn't Some(sp), just a help line similar to what we have now.

You can see how I handled a similar suggestion case in 28ea03e, but that one was more self contained.

@@ -5416,7 +5453,7 @@ impl<'a> Parser<'a> {
// or with mandatory equality sign and the second type.
let ty = self.parse_ty()?;
if self.eat(&token::Colon) {
let bounds = self.parse_generic_bounds()?;
let bounds = self.parse_generic_bounds(syntax_pos::DUMMY_SP)?;

This comment has been minimized.

@estebank

estebank Jan 14, 2019

Contributor

This makes me thing that either the argument should be an Option<Span>.

@hdhoang

This comment has been minimized.

Copy link
Contributor

hdhoang commented Jan 16, 2019

thanks! I intend to update this weekend, and see how it goes

@hdhoang hdhoang force-pushed the hdhoang:33418_negative_bounds branch from 3ad7e5d to a27cb59 Jan 19, 2019

@hdhoang

This comment has been minimized.

Copy link
Contributor

hdhoang commented Jan 19, 2019

hopefully I have addressed your review correctly. I have also downgraded the colon suggestion to a warning, since it's due to rustfix, not the original code

@hdhoang

This comment has been minimized.

Copy link
Contributor

hdhoang commented Jan 23, 2019

ping @estebank, have you had a chance chance to look at this?

@estebank
Copy link
Contributor

estebank left a comment

Apologies for the delay. Some suggestions inline. Let me know what you think.

":".into(),
Applicability::MachineApplicable)
.emit();
}

This comment has been minimized.

@estebank

estebank Jan 23, 2019

Contributor

As neat as the output is, it's unadvisable to have multiple suggestions that need to be applied together for them to be correct in separate diagnostics. A more appropriate solution would be to batch all the errors to be emitted (or at least the elements needed to create diagnostics) and emit a single, unambiguous diagnostic with the full suggestion. This would have the nice side effect of also dealing with Tr5 with a single diagnostic, instead of one per negative trait bound.

Do you feel this is reasonable?

"".into(),
Applicability::MachineApplicable);
} else {
err.help("remove this trait bound");

This comment has been minimized.

@estebank

estebank Jan 23, 2019

Contributor

If the other comment is implemented, this is the branch where you would have to account for the trailing + span instead, and suggest the removal of it, instead of in the warning.

LL | + !SuperB {} //~ ERROR negative trait bounds are not supported
| --^^^^^^^
| |
| help: remove this trait bound

This comment has been minimized.

@estebank

estebank Jan 23, 2019

Contributor

If the changes are implemented, this output would be:

error: negative trait bounds are not supported
  --> $DIR/issue-33418.rs:11:12
   |
LL | trait Tr5: !SuperA
   |            ^^^^^^^
LL |     + !SuperB {}
   |       ^^^^^^^
help: remove the trait bound:
   |
LL | trait Tr5 {}
   |         --
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment