Simplify Warnings #526

InsertCreativityHere · 2023-04-19T18:49:33Z

This PR implements #463 by adding the ability to suppress warnings by category, instead of requiring a warning code.

// all comment related warnings in this file will be suppressed.
[[allow(Comments)]]
module Test

There are 3 categories: All, Deprecated, and Comments. These arguments are case sensative and required.
Previously, writing the attribute with no arguments: [allow] said to allow all warnings. With these changes, it is an error to not supply an argument. If a user wants to suppress all warnings, they need to explicitly say so with: [allow(All)]

It also:

changes the -w command line argument to -W, as pointed out in Improve slice compiler help #517 (comment)
adds -A as a short option for --allow-warnings (currently we only allow the fully-written-out option)
Deletes the InvalidWarningCode error. Now we emit an ArgumentNotSupported error like we do for every other place a bad attribute argument is supplied. There's nothing special about the allow attribute that deserves its own error.
Removed an extra space character from our deprecation warning message.

`DocComment` vs `Comment` for the category name

Technically, since normal comments are completely ignored, only doc comments can cause warnings to be raised.
But saying Comments is still completely correct and easier to type, so I chose that as the category name.

src/command_line.rs

src/diagnostics/diagnostic_reporter.rs

src/diagnostics/warnings.rs

src/diagnostics/diagnostic_reporter.rs

src/grammar/elements/attribute.rs

src/diagnostics/diagnostic_reporter.rs

externl · 2023-04-20T17:59:40Z

I think this is still too complex. There's no need to have all these doc comment sub categories. I think we should do the following.

Get rid of the warning codes. No more WXXX codes.
Have the following (maybe we'll need a few more) warnings.
- all
- deprecated
- malformed_doc_comment

Maybe we need one or so more doc comments, but not nearly as many as we have now.

For 0.2 we can add missing_doc_comment, but it has to be enabled with a #[warn(missing_doc_comment)] or --warn missing_doc_comment (similar to rustc).

InsertCreativityHere · 2023-04-21T05:57:48Z

There's no need to have all these doc comment sub categories.

What do you mean by subcategories? We only have 3 categories and there are no subcategories.
I suspect there's a difference in terminology here?

Get rid of the warning codes. No more WXXX codes.

Why? What's wrong with providing fine grained warning suppression?
Also note there are some warnings that don't have a category, and can only be ignored by code currently.
For example: DuplicateFileError. It's not in any of the 3 categories, ignoring it requires using a code.

I'd be fine using human-readable names instead of warning codes, if that's what you're talking about though.

all
deprecated
malformed_doc_comment

These are the 3 categories that we have currently, albeit it with different names.
They can be lower-case on the command line, but every other attribute takes Pascal case arguments, so I'd rather stick with that. I'd also prefer to keep them to a single word so they're easier to type, and we don't need to worry about how to separate words. But if you strongly prefer something like "MalformedDocComment" to "Comment", I wouldn't fight it.

externl · 2023-04-21T13:43:54Z

I suspect there's a difference in terminology here?
Yes. I was calling All, Comment, etc. categories and the WXXX warnings the sub-categories. To rephrase:

Keep warnings was the word we use
Replace the code by a keyword like Rust
No categories

I'd be fine using human-readable names instead of warning codes, if that's what you're talking about though.
Yes!

Why? What's wrong with providing fine grained warning suppression?

It's too fine grained. We just don't need to support all the ways you can miss parts of doc comments.

They can be lower-case on the command line

I was staring at Rust when I wrote my comment. They should indeed always be pascal case.

We need to decide on which warnings have. The name should also be something that makes sense when you used in context with allow(XXX) and warn(XXX).

For instance, allow(Comment) is confusing. allow(MalformedDocComment) makes more sense.

We currently have:

DuplicateFile 
DocCommentSyntax   
ExtraParameterInDocComment   
ExtraReturnValueInDocComment
ExtraThrowInDocComment
CouldNotResolveLink 
LinkToInvalidElement 
UseOfDeprecatedEntity 
InvalidThrowInDocComment 
OperationDoesNotThrow

I propose the following restructuring and renaming.

DuplicateFile
MalformedDocComment (includes un-resolveable doc links)
Deprecated

To add in 0.2 (default allow, use warn to enable`

MissingDocComment

Ones I would like to merge.

ExtraParameterInDocComment
ExtraReturnValueInDocComment
ExtraThrowInDocComment
InvalidThrowInDocComment
OperationDoesNotThrow

Into two:

IncompleteDocComment - add in 0.2. allow by default
InconsistentDocComment - warn by default

pepone · 2023-04-21T13:56:48Z

Get rid of the warning codes. No more WXXX codes.

What does this mean? Will we still provide warning codes with the diagnostic? MSBuild logger expects these warning codes.

Do we really need this feature? it doesn't seem that useful to suppress categories of warnings.

externl · 2023-04-21T14:11:09Z

What does this mean? Will we still provide warning codes with the diagnostic? MSBuild logger expects these warning codes.

We just won't have a code like W123. Instead we'll have one like MalformedDocComment.

bernardnormier · 2023-04-21T14:15:43Z

You should open an issue. It's hard to keep track of proposals hidden in PR review comments!

externl · 2023-04-21T14:36:29Z

You should open an issue. It's hard to keep track of proposals hidden in PR review comments!

It's a comment directly related to the content of the PR. I can comment on the original issue. We can open an issue for future improvements after this PR.

InsertCreativityHere · 2023-04-24T21:18:38Z

Keep warnings was the word we use 👍

Replace the code by a keyword like Rust 👍

Fine with me, but will this work with the builder @pepone? Can it take any string as a code, or does it require a specific format?

No categories

I'm guessing this is another terminological difference here, but then what do you propose calling them?
Because to me: MalformedDocComment feels like a category or warnings.

For instance, allow(Comment) is confusing. allow(MalformedDocComment) makes more sense. 👍

Do you think that specifying Doc is necessary? I'd slightly prefer MalformedComment, because it's already a pretty long name,
and the shorter the better.

Ones I would like to merge.

What do you mean by merge? Do you mean like, actually merge the WarningKind enumerators together into a single enumerator? If so, I'll probably put that off for another PR.

Why? What's wrong with providing fine grained warning suppression?

It's too fine grained. We just don't need to support all the ways you can miss parts of doc comments.

Okay, but why? It's trivial to implement, and seems useful to me.
Maybe I don't want to use Slice's fancy doc comment syntax for returns, throws, etc. because plain text is good enough, BUT I do want it to generate links for me correctly.

I'm fine with the proposed categories, but I think it's still worth having an All category (in addition to the others proposed).
The entire point of this PR is:

We think it's okay for users to ignore warnings they don't care about
We should make it easier for users to ignore them (what this PR does)
Having an All category instead of requiring them to list out all the others (which isn't future proof either) seems easier.

Also, while I don't find this compelling, I'd note that supporting this is pretty standard for every major compiler.

pepone · 2023-04-24T22:00:14Z

Can it take any string as a code, or does it require a specific format?

Yes, any string will do.

bernardnormier · 2023-04-24T22:01:00Z

Do you think that specifying Doc is necessary? I'd slightly prefer MalformedComment, because it's already a pretty long name, and the shorter the better.

Short & clear is ok, short & confusing is not. What is a "malformed comment"? A comment with a non-ASCII character? I find this term very confusing.

InsertCreativityHere · 2023-04-25T14:10:44Z

Short & clear is ok, short & confusing is not. What is a "malformed comment"? A comment with a non-ASCII character? I find this term very confusing.

I agree, "malformed" is a slightly odd choice. Maybe we should use "invalid" instead?

InsertCreativityHere · 2023-04-25T14:13:12Z

It's too fine grained. We just don't need to support all the ways you can miss parts of doc comments.

Just to show that fine-grained has some precedence, rustdoc has specific warnings for broken links, broken code blocks, bad html tags, etc: https://doc.rust-lang.org/rustdoc/lints.html

externl · 2023-04-25T14:36:56Z

Short & clear is ok, short & confusing is not. What is a "malformed comment"? A comment with a non-ASCII character? I find this term very confusing.

I agree, "malformed" is a slightly odd choice. Maybe we should use "invalid" instead?

I think @bernardnormier was referring to "malformed doc comment" vs "malformed comment". The later doesn't make sense. You can't have malformed comment, but you can have a malformed doc comment.

InsertCreativityHere · 2023-04-26T22:02:38Z

EVERYTHING ABOVE THIS COMMENT IS OUTDATED NOW!

InsertCreativityHere · 2023-04-26T22:02:46Z

I reworked the PR from the ground up, so I marked all the old review comments as "Resolved" since they're inapplicable now.

This PR does the following:

Completely remove our numerical codes for warnings (W003) and replaces them with names (BrokenDocLink).
These new names are emitted in diagnostics and used by the allow attribute & command line option.
Previously, using the allow attribute or command line option without specifying any arguments would 'allow all warnings', now the user must explicitly write this: allow(All) or --allow All. It is an error not to supply arguments to these now.
Allowing DuplicateFile warnings with attributes is disallowed. It's a warning about improper command line arguments. It makes no sense to allow this outside the CLI. What would [allow(DuplicateFile)] struct S { ... } even mean??
Added validation of arguments to --allow passed on the command line. See: Add back ignore warning validation #431.
Deleted InvalidWarningCode error. There's nothing special about the allow attribute. Supplying an incorrect argument will trigger an ArgumentNotSupported error like every other attribute uses.

InsertCreativityHere · 2023-04-26T22:08:57Z

Now, slicec only has the following warnings:

DuplicateFile (you passed the same file in multiple times)
Deprecated (you used something marked 'deprecated')
MalformedDocComment (syntax error for doc comment)
BrokenDocLink (You link to a non-existent or non-linkable type)
IncorrectDocComment (Some of the information in a doc comment is wrong)

This is how the old warnings were mapped to the new ones:

DocCommentSyntax -> MalformedDocComment
UseOfDeprecatedEntity -> Deprecated
The message we emit wasn't changed, but I simplified how we store the strings we generate it from.
OperationDoesNotThrow -> IncorrectDocComment
ExtraParameterInDocComment -> IncorrectDocComment
InvalidThrowInDocComment -> IncorrectDocComment
ExtraReturnValueInDocComment -> IncorrectDocComment
ExtraThrowInDocComment -> IncorrectDocComment
CouldNotResolveLink -> BrokenDocLink
I also changed the message:
OLD: no element with identifier '{identifier}' can be found from this scope
NEW: no element named '{identifier}' exists in scope
LinkToInvalidElement -> BrokenDocLink
I also changed the message:
OLD: elements of the type '{kind}' cannot be referenced in doc comments
NEW: {kind}s cannot be linked to

InsertCreativityHere · 2023-04-26T22:13:54Z

I opened an issue for adding warnings for missing doc comment things in 0.2: #537

bernardnormier · 2023-04-27T00:02:03Z

I assume BrokenLink is purely about doc-comments?

We don't care about links in regular comments. And for actual Slice definitions, a broken reference (like Foo::Bar) is obviously an error. Do you think this will be obvious to our users?

InsertCreativityHere · 2023-04-27T01:43:54Z

I assume BrokenLink is purely about doc-comments?

Yes. Non-doc-comments are completely ignored, so even if you type the syntax of a link, it has no semantic meaning.
And referencing a Slice type outside of a doc comment is something completely different.

Do you think this will be obvious to our users?

I think "link" is clear enough, and that most of our users wouldn't consider "referencing a type" to be a "link".
But "obvious" is a high bar. I'd be fine naming it "BrokenDocLink" if you think it necessary.

InsertCreativityHere · 2023-04-27T02:36:16Z

I think we should rename it BrokenDocLink.
It seems more consistent with all of our other warnings that include the word Doc.

pepone · 2023-04-27T08:14:21Z

I think we should rename it BrokenDocLink.

Or maybe BrokenDocReference

externl · 2023-04-27T14:50:11Z

I think we should rename it BrokenDocLink.

Or maybe BrokenDocReference

Link is the keyword used in the doc comments themselves so I'd be inclined to use it in the warning name.

src/diagnostics/diagnostic_reporter.rs

src/validators/comments.rs

externl · 2023-04-27T15:05:34Z

src/validators/comments.rs

-                    .set_scope(operation.parser_scoped_identifier())
-                    .report(diagnostic_reporter);
+                Diagnostic::new(Warning::IncorrectDocComment {
+                    message: "void operation must not contain doc comment return tag".to_owned(),


I don't think would use the phrase "void operation". What about something like:

doc comment contains a return tag but the operation does not have a return value

I know that I changed 2 messages in this PR, but I think I'm just going to open an issue to check the others.
This PR was originally supposed to be about adding warning categories, and it's already come pretty far from those tracks.

tests/attribute_tests.rs

externl · 2023-04-27T15:10:11Z

tests/attribute_tests.rs

+        #[test_case("Deprecated", [1, 2]; "deprecated")]
+        #[test_case("BrokenLink", [0, 2]; "broken_link")]
+        #[test_case("IncorrectDocComment", [0, 1]; "incorrect_doc_comment")]
+        fn allow_only_specified_warnings<const L: usize>(arguments: &str, expected_indexes: [usize; L]) {


I find this test confusing. What is it testing?

It's testing that only the correct warnings are emitted.
So for instance allowing Deprecated shouldn't have an effect on warnings unrelated to it.
It does this by having an array of 3 warnings, and each test case says what warnings should be emitted (by index).

I can split this into 3 tests, write a doc comment, or something else. Let me know what you think is best.

Co-authored-by: Joe George <joe@externl.com>

InsertCreativityHere · 2023-04-27T18:16:13Z

I renamed BrokenLink to BrokenDocLink and updated my comments to match this.

src/command_line.rs

src/diagnostics/diagnostic_reporter.rs

tests/attribute_tests.rs

src/diagnostics/mod.rs

externl · 2023-05-01T14:48:25Z

@InsertCreativityHere the actions are failing still!

InsertCreativityHere added 2 commits April 18, 2023 18:01

Added warning categories.

d43fd00

Removed attribute warning category.

d185100

InsertCreativityHere requested review from externl and ReeceHumphreys April 19, 2023 18:49

Minor improvements.

e63ff55

externl reviewed Apr 19, 2023

View reviewed changes

InsertCreativityHere added 7 commits April 25, 2023 11:16

Merge branch 'main' into add-warning-categories

1c3d5ac

Removed warning categories.

f5af475

Added warning code validation.

08500a0

Removed numerical warning codes.

b93dfa9

Fixed a bug in my warning filtering logic.

88bb7af

Reimplemented everything.

b4a2649

Made the spell checker happy.

5c534a2

InsertCreativityHere changed the title ~~Added Warning Suppression Categories~~ Simplify Warnings Apr 26, 2023

Renamed 'allow-warning' to 'allow'.

9e15afd

InsertCreativityHere requested review from externl, bernardnormier and ReeceHumphreys and removed request for ReeceHumphreys April 26, 2023 22:09

externl reviewed Apr 27, 2023

View reviewed changes

InsertCreativityHere and others added 4 commits April 27, 2023 13:37

Fixes.

dfa95c6

Update tests/attribute_tests.rs

5dcf0fe

Co-authored-by: Joe George <joe@externl.com>

Update tests/attribute_tests.rs

9c34201

Co-authored-by: Joe George <joe@externl.com>

'BrokenLink' -> 'BrokenDocLink'.

33cccf6

ReeceHumphreys reviewed May 1, 2023

View reviewed changes

src/command_line.rs Show resolved Hide resolved

src/diagnostics/diagnostic_reporter.rs Show resolved Hide resolved

tests/attribute_tests.rs Show resolved Hide resolved

tests/attribute_tests.rs Show resolved Hide resolved

src/diagnostics/mod.rs Show resolved Hide resolved

ReeceHumphreys approved these changes May 1, 2023

View reviewed changes

InsertCreativityHere added 2 commits May 8, 2023 09:54

Merge remote-tracking branch 'origin' into add-warning-categories

bd38bde

Fixed error introduced by merge.

cf068a3

InsertCreativityHere merged commit feb685a into icerpc:main May 8, 2023

This was referenced May 8, 2023

Add back ignore warning validation #431

Closed

warning regression #463

Closed

Intercept Command Line Errors from Clap #429

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify Warnings #526

Simplify Warnings #526

InsertCreativityHere commented Apr 19, 2023

externl commented Apr 20, 2023

InsertCreativityHere commented Apr 21, 2023

externl commented Apr 21, 2023 •

edited

Loading

pepone commented Apr 21, 2023

externl commented Apr 21, 2023 •

edited

Loading

bernardnormier commented Apr 21, 2023 •

edited

Loading

externl commented Apr 21, 2023 •

edited

Loading

InsertCreativityHere commented Apr 24, 2023

pepone commented Apr 24, 2023

bernardnormier commented Apr 24, 2023

InsertCreativityHere commented Apr 25, 2023

InsertCreativityHere commented Apr 25, 2023

externl commented Apr 25, 2023

InsertCreativityHere commented Apr 26, 2023

InsertCreativityHere commented Apr 26, 2023 •

edited

Loading

InsertCreativityHere commented Apr 26, 2023 •

edited

Loading

InsertCreativityHere commented Apr 26, 2023

bernardnormier commented Apr 27, 2023

InsertCreativityHere commented Apr 27, 2023

InsertCreativityHere commented Apr 27, 2023

pepone commented Apr 27, 2023

externl commented Apr 27, 2023 •

edited

Loading

externl Apr 27, 2023

InsertCreativityHere Apr 27, 2023

externl Apr 27, 2023

InsertCreativityHere Apr 27, 2023

InsertCreativityHere commented Apr 27, 2023

externl commented May 1, 2023

Simplify Warnings #526

Simplify Warnings #526

Conversation

InsertCreativityHere commented Apr 19, 2023

DocComment vs Comment for the category name

externl commented Apr 20, 2023

InsertCreativityHere commented Apr 21, 2023

externl commented Apr 21, 2023 • edited Loading

pepone commented Apr 21, 2023

externl commented Apr 21, 2023 • edited Loading

bernardnormier commented Apr 21, 2023 • edited Loading

externl commented Apr 21, 2023 • edited Loading

InsertCreativityHere commented Apr 24, 2023

pepone commented Apr 24, 2023

bernardnormier commented Apr 24, 2023

InsertCreativityHere commented Apr 25, 2023

InsertCreativityHere commented Apr 25, 2023

externl commented Apr 25, 2023

InsertCreativityHere commented Apr 26, 2023

EVERYTHING ABOVE THIS COMMENT IS OUTDATED NOW!

InsertCreativityHere commented Apr 26, 2023 • edited Loading

InsertCreativityHere commented Apr 26, 2023 • edited Loading

InsertCreativityHere commented Apr 26, 2023

bernardnormier commented Apr 27, 2023

InsertCreativityHere commented Apr 27, 2023

InsertCreativityHere commented Apr 27, 2023

pepone commented Apr 27, 2023

externl commented Apr 27, 2023 • edited Loading

externl Apr 27, 2023

Choose a reason for hiding this comment

InsertCreativityHere Apr 27, 2023

Choose a reason for hiding this comment

externl Apr 27, 2023

Choose a reason for hiding this comment

InsertCreativityHere Apr 27, 2023

Choose a reason for hiding this comment

InsertCreativityHere commented Apr 27, 2023

externl commented May 1, 2023

`DocComment` vs `Comment` for the category name

externl commented Apr 21, 2023 •

edited

Loading

externl commented Apr 21, 2023 •

edited

Loading

bernardnormier commented Apr 21, 2023 •

edited

Loading

externl commented Apr 21, 2023 •

edited

Loading

InsertCreativityHere commented Apr 26, 2023 •

edited

Loading

InsertCreativityHere commented Apr 26, 2023 •

edited

Loading

externl commented Apr 27, 2023 •

edited

Loading