Diagnostic when trying to use ! or ? in trait is confusing.

[vi]

trait Qqq : !Sized {
}

error: expected one of `?`, `where`, or `{`, found `!`

OK, let's try ?:

trait Qqq : ?Sized {
}

error: unexpected `?`

Why were you expecting ? then?

[durka]

The unhelpful answer is "because the same code is used to parse generic type parameters and this part of a trait declaration, so the error isn't detected until we're in too deep"... but it should just give a better message from the beginning.

[Mark-Simulacrum]

Unfortunately, this hasn't improved much since then, though the unexpected ? message is now much better. I'm also not sure what a good message would be for the first case--does anyone have any suggestions so that we can make this bug more actionable?

$ rustc first-example.rs
error: expected one of `(`, `?`, `for`, `where`, `{`, lifetime, or path, found `!`
 --> test.rs:1:13
  |
1 | trait Qqq : !Sized {
  |            -^ unexpected token
  |            |
  |            expected one of 7 possible tokens here

error: aborting due to previous error

$ rustc second-example.rs
error: `?Trait` is not permitted in supertraits
 --> test.rs:1:14
  |
1 | trait Qqq : ?Sized {
  |              ^^^^^
  |
  = note: traits are `?Sized` by default

error: main function not found

error: aborting due to 2 previous errors

[durka]

Is "negative trait bounds are not supported" too straightforward?

…

On Wed, May 3, 2017 at 7:07 PM, Mark Simulacrum ***@***.***> wrote: Unfortunately, this hasn't improved much since then, though the unexpected ? message is now much better. I'm also not sure what a good message would be for the first case--does anyone have any suggestions so that we can make this bug more actionable? $ rustc first-example.rs error: expected one of `(`, `?`, `for`, `where`, `{`, lifetime, or path, found `!` --> test.rs:1:13 | 1 | trait Qqq : !Sized { | -^ unexpected token | | | expected one of 7 possible tokens here error: aborting due to previous error $ rustc second-example.rs error: `?Trait` is not permitted in supertraits --> test.rs:1:14 | 1 | trait Qqq : ?Sized { | ^^^^^ | = note: traits are `?Sized` by default error: main function not found error: aborting due to 2 previous errors — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#33418 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAC3n2Cn6pOUJmk4MNh9Ikv6Rrq2EA3Oks5r2QiZgaJpZM4IXqsq> .

[Mark-Simulacrum]

That would work. Thanks for the suggestion!

[hdhoang]

Hello! I would like to implement this improvement.

AIUI, we would like to improve the parse error message for negative supertrait bounds, but negative normal bounds on types are OK.

First we add a new parser test as ui/parser/issue-33418.rs:

trait Tr: !Sized {} //~ ERROR Negative trait bounds are not supported

and for bounds after +:

trait Tr: Send + !Sized {} //~ ERROR Negative trait bounds are not supported

Then we have some code to emit a custom error when encountering the unexpected !:

if self.check(&token::Not) {
    self.span_err(self.span, "Negative trait bounds are not supported");
}

Now I have some trouble finding where to place this check. The closest parsing section is in fn parse_item_trait:

5917   │         // Parse optional colon and supertrait bounds.
5918   │         let bounds = if self.eat(&token::Colon) {
5919   │             self.parse_generic_bounds()?

However, inserting the check before line 5919 wouldn't catch the second test case (its ! isn't right after the colon).

Next I tried inserting it at the top of each iteration in fn parse_generic_bounds_common's loop, which is passing much of the default testsuite (it hasn't completed yet, takes a long while on my machine). I find this surprising since this function covers generic parsing, and the new check should fail fn<T: !Unpin> foo(_t: T) and similar. Please check my understanding here.

I have put my work so far into https://github.com/hdhoang/rust/tree/33418_negative_bounds.

[estebank]

Looking at this, @hdhoang, it seems to me that the "correct" way of dealing with this would be to add a new TraitBoundModifier::Bang that is only used for error reporting, and parse it similarly to how we parse TraitBoundModifier::Maybe. The problem with this is that we would have to carry a GenericBound::Trait(ref poly, TraitBoundModifier::Bang) around that would be completely artificial purely for error reporting.

That being said, if the amount of code being changed to add this were small and the variant were called something along the lines of TraitBoundModifier::BangForErrorReporting, I'd think it'd be a reasonable addition. After adding it, you could just generate a targeted error in no_questions_in_bounds with a suggestion to simply remove the ! or use ? with an explanation of the difference between ?Trait and Trait. This also means that anywhere where TraitBoundModifier::None is expected could also consider BangForErrorReporting as correct (or ignore it, where appropriate) to minimize downstream errors.

[hdhoang]

Thank you for your guidance! I will try to work this out.

Do you have any suggestion for run-pass tests and similar to avoid unintended breakage?

[hdhoang]

I have implemented & pushed your method. It did lead to a better experience with the tests!

I have only added a small note to remove the whole negative bound. I will need some more time to target !Sized in particular with a specific message guiding towards ?Sized.

[estebank]

Looked at your branch and it already looks quite nice! A couple of nitpicks before you post a PR:

https://github.com/hdhoang/rust/blob/25f7ddc42365bb0a27833797a2eec8172b7b56ec/src/libsyntax/parse/parser.rs#L5086

You're not actually using the !'s span, so you can just store a boolean there.

https://github.com/hdhoang/rust/blob/25f7ddc42365bb0a27833797a2eec8172b7b56ec/src/test/ui/parser/issue-33418.stderr

This is already looking great! You can use err.span_suggestion_with_applicability to provide a structured suggestion (that VSCode and rustfix can apply automatically), but for the suggestion to apply cleanly without resulting in incorrect code, the span used should also include the prior : or +, so when they are removed the code still compiles. This span could be carried in TraitBoundModifier::ErroneousNegative (but that would increate the size of TraitBoundModifier, so I'd like to do a timer run before merging the PR to make sure it's not significant).

Other than that, great job!

[hdhoang]

I have simplified the ! span, and made preparation for a code suggestion. In a separate commit, I have also tried to muck around in the source_map to build a machine-applicable suggestion, but I'm not sure how it will interact with generated code or the like. Hopefully this will keep the happy path fast.

Could you elaborate on the ! and ? suggestions?

or use ? with an explanation of the difference between ?Trait and Trait

Won't this lead back to the "?Trait is not permitted in supertraits"?

Furthermore, I would like to be consistent with the terminology to help with rging through the code. How do you like the optional parts in "negative (super( )?)?trait(s)? bound(s)?", in both the messages and code comment?