Revamp the support for "future incompatible" lints #30787

nikomatsakis · 2016-01-08T22:56:06Z

There is now more structure to the report, so that you can specify e.g. an RFC/PR/issue number and other explanatory details.

Example message:

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters are only allowed on type definitions, like `struct` or `enum`
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:14:8: 14:9 warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
type-parameter-invalid-lint.rs:14:8: 14:9 note: for more information, see PR 30742 <https://github.com/rust-lang/rust/pull/30724>
type-parameter-invalid-lint.rs:11:9: 11:28 note: lint level defined here
type-parameter-invalid-lint.rs:11 #![deny(future_incompatible)]
                                          ^~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

r? @brson

I would really like feedback also on the specific messages!

Fixes #30746

nagisa · 2016-01-08T22:59:07Z

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters are only allowed on struct or enum definitions (see issue #27336)
type-parameter-invalid-lint.rs:14:8: 14:9 note: type parameter defaults were not intended to be permitted outside of types (see pull request #30724)

IMO “see *” should be a HELP or HINT.

type-parameter-invalid-lint.rs:14:8: 14:9 note: this lint will become a HARD ERROR in a future release!

And this – a warning.

solson · 2016-01-08T23:01:00Z

Can we include the actual URL for issues and PRs?

brson · 2016-01-08T23:45:44Z

OK, as I understand it, this adds additional metadata for future-incompatible lints, resulting in 3 lines displayed, the 'normal' lint message, the future-incompatible explanation with either PR/issue/RFC, and a boilerplate HARD ERROR warning.

I'm a bit confused about the exabmple in the OP. It looks to me like it was not generated with the commit in the PR.

In particular:

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters are only allowed on `struct` or `enum` definitions (see issue #27336)
type-parameter-invalid-lint.rs:14:8: 14:9 note: type parameter defaults were not intended to be permitted outside of types (see pull request #30724)

I think the errors described are two separate future-incompatible errors, but the first is an error' and the second is a note which makes me think the two are connected. Based on the code I expected to see an error with no issue number, then an extended note with an error number.

Based on what I see here and what I think I understand of this PR I might suggest the following tweak:

Combine the 'normal' lint description and the future-incompatible explanation into one, but add the RFC/issue/PR number as FutureIncompatibleInfo.
Make the first error the normal error message
Make the second note the message "this will become a HARD ERROR in a future release!"
Make the third "for more information see PR #XXXX, at url {url}".

Combining the two custom messages reduces duplication. With that and the expanded note for the URL I feel like switching the order of the 'hard error' and url notes. I think it could still be more clear that 1) this is a new error for a pattern that used to be valid, 2) it's breaking is part of an intentional transition strategy. I couldn't come with a simple and clear way to say those though.

I do think we should spring for the URL, do everything we can to help hapless users - they may not know where to look for RFCs e.g.

nikomatsakis · 2016-01-09T00:34:31Z

@brson those messages were indeed generated by this code and are all connected. Right now there 3 pieces:

the lint message itself
the "extra info" that was intended to be explaining why you are getting this lint now
the HARD ERROR notice

I agree it's too much. I also had a hard time though trying to pithily explain:

this is a new error for a pattern that used to be valid, 2) it's breaking is part of an intentional transition strategy.

nikomatsakis · 2016-01-09T00:35:07Z

So it seems like you are suggesting:

Remove the explanatory message and just put that in the lint error message itself.
Experiment with phrasing to try and convey everything better.

I'd love any suggestions as to good wordings ;)

nikomatsakis · 2016-01-09T01:46:17Z

@nagisa unfortunately I can't make the "HARD ERROR" part a warning -- or at least not easily -- because the diagnostic builder doesn't support that. You can only append notes, helps, etc.

nikomatsakis · 2016-01-09T01:46:42Z

Hmm, but that might be easy to change :)

nikomatsakis · 2016-01-09T03:23:16Z

OK, here is the newer output.

For the invalid type parameter case:

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters were only intended to be allowed on `struct` or `enum` definitions
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:14:8: 14:9 warning: this was fixed in Rust 1.7; it will become a HARD ERROR in a future release!
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:14:8: 14:9 note: for more information, see https://github.com/rust-lang/rust/pull/30724
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:11:9: 11:28 note: lint level defined here
type-parameter-invalid-lint.rs:11 #![deny(future_incompatible)]
                                          ^~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

For private types in public APIs:

private-in-public-warn.rs:220:20: 220:45 warning: private trait in public interface (error E0445), #[warn(private_in_public)] on by default
private-in-public-warn.rs:220     pub trait Tr2: PrivUseAliasTr<PrivAlias> {} //~ WARN private trait in public interface
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
private-in-public-warn.rs:220:20: 220:45 warning: this was fixed in Rust 1.7; it will become a HARD ERROR in a future release!
private-in-public-warn.rs:220     pub trait Tr2: PrivUseAliasTr<PrivAlias> {} //~ WARN private trait in public interface
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~
private-in-public-warn.rs:220:20: 220:45 note: for more information, see the explanation for E0446 (`--explain E0446`)
private-in-public-warn.rs:220     pub trait Tr2: PrivUseAliasTr<PrivAlias> {} //~ WARN private trait in public interface
                                                 ^~~~~~~~~~~~~~~~~~~~~~~~~

bors · 2016-01-11T15:29:47Z

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

brson · 2016-01-12T02:15:55Z

@brson those messages were indeed generated by this code and are all connected. Right now there 3 pieces:

Maybe the thing that confused me was the first error message contained an issue number, while the future-incompatible explanation also contained a PR #. The error message issue # I think must have been just hardcoded in the message string, which is weird when compared to the PR # that has its own field in the extended future-incompatible data.

Remove the explanatory message and just put that in the lint error message itself.

Well, after reading your example again I'm not sure. As you've phrased it it does make sense to have two sentences at least. The first says what the error is, the second says why. I guess what I was thinking was that the 'why' could just be a standard 'this is a future-incompatible error, for more info see...' message and not try to justify it inline.

brson · 2016-01-12T02:28:54Z

I like the new iteration better, I think, but "defaults for type parameters were only intended to be allowed on struct or enum definitions" is kind of a weird error. The 'were only intended' doesn't give very strong indication of who did wrong. Your previous error of 'defaults for type parameters are only allowed on struct or enum definitions' gives a better indication that the user has done something wrong.

In this example:

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters were only intended to be allowed on `struct` or `enum` definitions
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:14:8: 14:9 warning: this was fixed in Rust 1.7; it will become a HARD ERROR in a future release!

Is this lint emitting an error because of #[deny(..)]? As emitted it reads weird because it says 'it will become a hard error', but it was a hard error.

Which brings up the question to me of what happens to these after they do become hard errors. I would hope that they continue carrying this metadata, saying 'this used to work, but doesn't now because {URL}', for those that skipped a few cycles.

Here's another attempt at writing the three lines about first error:

error: defaults for type parameters are only allowed on `struct` or `enum` definitions
warning: this was previously valid Rust but is being phased out, and will become a HARD ERROR in a future release!
note: for more information, see https://github.com/rust-lang/rust/pull/30724

It says what the error is (not why); that you aren't crazy, it used to work, but will not work soon; see link for further info.

petrochenkov · 2016-01-12T06:37:11Z

src/librustc_typeck/collect.rs

-                format!("defaults for type parameters are only allowed \
-                         on `struct` or `enum` definitions (see issue #27336)"));
+                format!("defaults for type parameters were only intended \
+                         to be allowed on `struct` or `enum` definitions"));


struct or enum?
what about type, trait, impl? They all don't generate the warning currently.

nikomatsakis · 2016-01-12T14:44:54Z

@brson

Is this lint emitting an error because of #[deny(..)]? As emitted it reads weird because it says 'it will become a hard error', but it was a hard error.

It is an error because of Deny. Good point though, I could adjust the wording in the case where the lint IS an error slightly.

nikomatsakis · 2016-01-12T14:45:52Z

@brson

error: defaults for type parameters are only allowed on `struct` or `enum` definitions
warning: this was previously valid Rust but is being phased out, and will become a HARD ERROR in a future release!
note: for more information, see https://github.com/rust-lang/rust/pull/30724

seems good.

nikomatsakis · 2016-01-12T14:46:16Z

@petrochenkov

struct or enum? what about type, trait, impl? They all don't generate the warning currently.

true, I will rephrase.

nikomatsakis · 2016-01-12T14:46:54Z

@brson

warning: this was previously valid Rust but is being phased out, and will become a HARD ERROR in a future release!

Do you think it is worth saying when it was changed? Maybe it doesn't matter.

nikomatsakis · 2016-01-12T17:36:09Z

I am inclined to change the wording of "previously valid Rust" to "previously accepted by the compiler", since frequently these fixes are the results of compiler bugs, and only very infrequently are they a genuine shift in policy.

better

nikomatsakis · 2016-01-12T18:51:09Z

OK, revised per suggestions. The current output:

type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters are only allowed on type definitions, like `struct` or `enum`
type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {}
                                         ^
type-parameter-invalid-lint.rs:14:8: 14:9 warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release!
type-parameter-invalid-lint.rs:14:8: 14:9 note: for more information, see PR 30742 <https://github.com/rust-lang/rust/pull/30724>
type-parameter-invalid-lint.rs:11:9: 11:28 note: lint level defined here
type-parameter-invalid-lint.rs:11 #![deny(future_incompatible)]
                                          ^~~~~~~~~~~~~~~~~~~
error: aborting due to previous error

Do you guys think the all caps HARD ERROR is too much? It was suggested I make it bold, which I would like to do, but not in this PR, because that would require adding some kind of rich formatting business.

UPDATE: Edited example to use fileline_span
UPDATE: Edited to remove caps.
UPDATE: incorporated new punctuation

nikomatsakis · 2016-01-12T18:51:21Z

(ping @brson to take a look again)

brson · 2016-01-15T00:31:03Z

Do you think it is worth saying when it was changed? Maybe it doesn't matter.

I do, but I also want it to be concise.

brson · 2016-01-15T00:32:36Z

@bors r+

bors · 2016-01-15T00:32:36Z

📌 Commit 0704279 has been approved by brson

@brson

…nt, r=brson There is now more structure to the report, so that you can specify e.g. an RFC/PR/issue number and other explanatory details. Example message: ``` type-parameter-invalid-lint.rs:14:8: 14:9 error: defaults for type parameters are only allowed on type definitions, like `struct` or `enum` type-parameter-invalid-lint.rs:14 fn avg<T=i32>(_: T) {} ^ type-parameter-invalid-lint.rs:14:8: 14:9 warning: this was previously accepted by the compiler but is being phased out; it will become a hard error in a future release! type-parameter-invalid-lint.rs:14:8: 14:9 note: for more information, see PR 30742 <rust-lang#30724> type-parameter-invalid-lint.rs:11:9: 11:28 note: lint level defined here type-parameter-invalid-lint.rs:11 #![deny(future_incompatible)] ^~~~~~~~~~~~~~~~~~~ error: aborting due to previous error ``` r? @brson I would really like feedback also on the specific messages! Fixes rust-lang#30746

rust-highfive assigned brson Jan 8, 2016

petrochenkov reviewed Jan 12, 2016
View reviewed changes

nikomatsakis added 2 commits January 12, 2016 12:53

Revamp the "future incompatible" section to clarify the situation

2593794

better

Fix test cases accordingly

7098cfd

nikomatsakis force-pushed the future-incompatible-lint branch from eaf533b to 7098cfd Compare January 12, 2016 18:49

nikomatsakis added 2 commits January 12, 2016 17:12

Fix long line in URL

f9b5463

use fileline_ and not full span_ for the followon messages

0254f69

nikomatsakis force-pushed the future-incompatible-lint branch from a0ca6e6 to 0254f69 Compare January 13, 2016 01:48

nikomatsakis added 2 commits January 13, 2016 10:11

s/HARD ERROR/hard error/ -- perhaps the warning is enough

5580b90

improve use of , vs ;

0704279

bors merged commit 0704279 into rust-lang:master Jan 15, 2016

nikomatsakis deleted the future-incompatible-lint branch March 30, 2016 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Revamp the support for "future incompatible" lints #30787

Revamp the support for "future incompatible" lints #30787

nikomatsakis commented Jan 8, 2016

nagisa commented Jan 8, 2016

solson commented Jan 8, 2016

brson commented Jan 8, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

bors commented Jan 11, 2016

brson commented Jan 12, 2016

brson commented Jan 12, 2016

petrochenkov Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

brson commented Jan 15, 2016

brson commented Jan 15, 2016

bors commented Jan 15, 2016

Revamp the support for "future incompatible" lints #30787

Revamp the support for "future incompatible" lints #30787

Conversation

nikomatsakis commented Jan 8, 2016

nagisa commented Jan 8, 2016

solson commented Jan 8, 2016

brson commented Jan 8, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

nikomatsakis commented Jan 9, 2016

bors commented Jan 11, 2016

brson commented Jan 12, 2016

brson commented Jan 12, 2016

petrochenkov Jan 12, 2016

Choose a reason for hiding this comment

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

nikomatsakis commented Jan 12, 2016

brson commented Jan 15, 2016

brson commented Jan 15, 2016

bors commented Jan 15, 2016