Misleading constant diagnostics #18933

elijah6 · 2017-04-23T09:33:23Z

Bugs this fixes:

#18750

Replaced WRN_FilterIsConstant warning with three separate warnings with different suggestions.

WRN_FilterIsConstantTrue: suggest removing the filter
WRN_FilterIsConstantFalse: suggest removing the catch clause
WRN_FilterIsConstantRedundantTryCatch: suggest removing the try-catch block, if the catch filter expression is 'false'/'default' and there is only one catch clause for the try-catch block.

elijah6 · 2017-04-23T10:55:02Z

It seems there was a memory issue with perf_correctness_prtest. Is it possible to re-run?

BattleRush · 2017-04-23T12:00:20Z

retest perf_correctness_prtest please

not sure if only members can use this command, but writing "test perf_correctness_prtest please" or "retest perf_correctness_prtest please" as a comment in the pull request should trigger a new run

elijah6 · 2017-04-23T12:08:06Z

@BattleRush Thank you! It doesn't seem to have been triggered, so I will try the comment on its own.

elijah6 · 2017-04-23T12:08:31Z

retest perf_correctness_prtest please

elijah6 · 2017-04-23T14:10:45Z

retest windows_release_vs-integration_prtest please

sharwell · 2017-04-23T14:52:58Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1232,17 +1232,20 @@ internal enum ErrorCode
        ERR_FixedBufferTooManyDimensions = 7092,
        ERR_CantReadConfigFile = 7093,
        ERR_BadAwaitInCatchFilter = 7094,
-        WRN_FilterIsConstant = 7095,
+        // WRN_FilterIsConstant = 7095,   // replaced by FilterIsConstantFalse and FilterIsConstantTrue


💭 It seems like FilterIsConstantTrue should still use this ID.

Sure, I will keep the ID value 7095, and use the new name FilterIsConstantTrue.
My thinking was to keep related warnings close together, but I guess keeping the orignal warning ID for the sake of backward compatibility (for warning disable pragma) is more important.

sharwell · 2017-04-23T14:57:35Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

        ERR_EncNoPIAReference = 7096,
        //ERR_EncNoDynamicOperation = 7097,   // dynamic operations are now allowed
        ERR_LinkedNetmoduleMetadataMustProvideFullPEImage = 7098,
        ERR_MetadataReferencesNotSupported = 7099,
        ERR_InvalidAssemblyCulture = 7100,
        ERR_EncReferenceToAddedMember = 7101,
        ERR_MutuallyExclusiveOptions = 7102,
+        WRN_FilterIsConstantFalse = 7103,
+        WRN_FilterIsConstantTrue = 7104,
+        WRN_FilterIsConstantRedundantTryCatch = 7105,


💭 I question the value of this specifically.

Between FilterIsConstantFalse and FilterIsConstantRedundantTryCatch, I don't foresee developers getting confused by the former but suddenly knowing how to proceed with the latter.

Distinguishing the cases introduces additional complexity in the compiler.

Distinguishing the cases introduces maintainability overhead since upgrade scenarios consider cases where warnings are treated as errors.

It boils down to paying a cost (complexity, maintainability) for a feature that no one will actually need in the end.

💡 If you keep this one, it should probably be renamed to WRN_FilterIsConstantFalseRedundantTryCatch (added False).

Thank you, @sharwell .
Should I then just distinguish True and False cases, and suggest consider removing the catch clause for all False cases? I agree it would reduce unnecessary overhead for the two false cases.
I will wait for additional comment from @gafter who originally raised #18750

Hi @gafter , do you have any preference/suggestion?

I'm not sure why we report this warning in the first place. Who is helped by this?

The answer to that question would set the direction of this pull request (may need to close and have a new one to remove the filter altogether?). I doubt this kind of code will be written in a practical application, but still seems like a nice hint if #19294 is to be merged as it skips redundant catch clause emission if unreachable.
Also, I am trying to think if any sort of automatic refactoring done by IDE could potentially write a dummy boolean value in the filter expression. In that case, if the developer forgets to replace the dummy value, it would at least come up as a warning in compilation.

sharwell · 2017-04-23T14:59:48Z

By the way, welcome to the project @elijah6 ! 🎉

Keep in mind I am not the expert in this section of the code (far from it). That's why I marked my comments with 💭 ... they show what I was thinking but it would make sense to wait for someone like @gafter or @jaredpar to provide additional details/comments before taking action based on something I said.

sharwell · 2017-05-09T18:59:01Z

@elijah6 I know a bunch of the best qualified reviewers for changes like this are busy preparing for Build. Even though it's not my area, I like what you're getting done here. I'm tracking this to try and get follow-up on it in a timely manner.

gafter · 2017-05-09T21:16:32Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                        : ErrorCode.WRN_FilterIsConstantFalse;
+
+                // Since the expression is a constant, the name can be retrieved from the first token
+                Error(diagnostics, errorCode, filter.FilterExpression, boundFilter.Syntax.GetFirstToken().Text);                


No, the name true is not available from the first token of (1 + 1) = 2.

That is a good point... It might be best to use the constant value to say ...filter is constant 'false'... and ...filter is constant 'true'... instead of specifying the name (e.g. if default is used, then it may be more natural to report ...filter is constant 'false'... rather than ...filter is constant 'default'... since the compiler already knows what it evaluates to. Similarly for (1+1)==2 or (1+1)!=2, it would then report as true and false respectively.
No need to pass around the name either in that case.

gafter · 2017-05-09T21:17:38Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                // we suggest different actions
+                var errorCode = boundFilter.ConstantValue.BooleanValue
+                    ? ErrorCode.WRN_FilterIsConstantTrue
+                    : hasSingleCatch


Rather than passing around hasSingleCatch, I think it would be cleaner to compute it here (from filter.Parent.Parent).

sharwell · 2017-05-12T15:41:17Z

@gafter It seems like this was reasonably well-approached and hopefully low risk. Can you take a look today to see if it would be a candidate for 15.3, and if so let @elijah6 know what (if anything) you'd be looking for in order to call it complete?

sharwell · 2017-05-18T19:51:34Z

@elijah6 It looks like this one may get retargeted to 15.6, but that's fine. 🙂 Your other PR will make a very nice addition for 15.3. 👍

elijah6 · 2017-05-18T20:44:22Z

Sure! 😄 Thank you for tracking these.

gafter · 2017-09-25T22:42:36Z

@elijah6 Can you please bring this PR up-to-date with master?

elijah6 · 2017-10-01T16:33:09Z

@gafter branch brought up-to-date.

gafter · 2017-10-01T19:34:36Z

src/Compilers/CSharp/Portable/Binder/Binder_Statements.cs

+                var errorCode = boundFilter.ConstantValue.BooleanValue
+                    ? ErrorCode.WRN_FilterIsConstantTrue
+                    : (filter.Parent.Parent as TryStatementSyntax).Catches.Count == 1
+                        ? ErrorCode.WRN_FilterIsConstantFalseRedundantTryCatch


It would not be redundant if there is also a finally clause.

That is true. It should be FilterIsConstantFalse in that case. I will make the fix.

gafter · 2017-10-01T19:35:45Z

src/Compilers/CSharp/Portable/Errors/ErrorCode.cs

@@ -1241,9 +1241,11 @@ internal enum ErrorCode
        ERR_EncReferenceToAddedMember = 7101,
        ERR_MutuallyExclusiveOptions = 7102,
        ERR_InvalidDebugInfo = 7103,
+        WRN_FilterIsConstantFalse = 7104,


These are in the wrong section; they were not, as the #region title describes, introduced in C# 6. Please move it to near the bottom.

I put them in that section only because they are bug fixes for the error code 7095 ("FilterIsConstant") introduced in C# 6. Isn't that the right approach for error code bug fixes? If not, should it go in a new section at the bottom?

It will be easier for the software archaeologists if we mostly add to the end.

They have been moved to the bottom.

gafter · 2017-10-01T19:37:04Z

src/Compilers/CSharp/Test/Semantic/Semantics/SemanticErrorTests.cs

+                //         catch (A) when (false) { }
+                Diagnostic(ErrorCode.WRN_FilterIsConstantFalseRedundantTryCatch, "(1+1)!=2").WithLocation(10, 25));
+        }
+


I would like to see what happens when the filter is a constant, but it is not boolean. What errors occur? Can you please add a test?

In that case, it will be handled by ERR_NoImplicitConv diagnostic. It has its own tests actually.

And that causes these diagnostics not to be produced?

yeah. I have added a test case to make it clearer.

gafter · 2017-10-02T22:08:14Z

@dotnet/roslyn-compiler Can we please have a second review of this community submission?

elijah6 · 2017-11-13T21:11:37Z

retest windows_debug_unit32_prtest please

gafter · 2017-11-13T22:51:40Z

@elijah6 Thank you so much for the contribution! Sorry it took so long to be processed.

elijah6 · 2017-11-14T19:35:18Z

@gafter Thank you!

dnfclas added the cla-signed label Apr 23, 2017

sharwell reviewed Apr 23, 2017

View reviewed changes

sharwell added the Community The pull request was submitted by a contributor who is not a Microsoft employee. label Apr 23, 2017

gafter reviewed May 9, 2017

View reviewed changes

gafter self-assigned this May 9, 2017

gafter added Area-Compilers Bug Concept-Diagnostic Clarity The issues deals with the ease of understanding of errors and warnings. labels May 9, 2017

elijah6 force-pushed the misleadingConstantDiagnostics branch from a945044 to fc5be2f Compare May 10, 2017 07:04

elijah6 force-pushed the misleadingConstantDiagnostics branch from fc5be2f to 25b8fea Compare May 20, 2017 08:21

elijah6 force-pushed the misleadingConstantDiagnostics branch from 3e65f68 to ce80269 Compare October 1, 2017 15:10

gafter reviewed Oct 1, 2017

View reviewed changes

gafter added this to the 15.5 milestone Oct 1, 2017

elijah6 force-pushed the misleadingConstantDiagnostics branch from ce80269 to a487c48 Compare October 4, 2017 17:34

elijah6 added 16 commits November 12, 2017 16:10

Separating warning messages for constant filter expression

eb50836

Fix tests

e5921a7

Fix minor comment and test name

aad08b5

Fixes based on review comment

2917131

Test argument fix

89c28d9

Fix merge conflict

0efe5b2

fix error code after merge

51ef705

merge fix

5cdaf5c

Deal with try-catch with finally block and add test

83df4e2

fix condition

547ca40

move down the errors, add non-boolean constant filter test

4c654d2

add endregion comment

d6c1518

generate errorfacts again

f7f3de5

correct error codes again

5397287

yet another error code change after rebase

f0dee13

And again

9090635

elijah6 force-pushed the misleadingConstantDiagnostics branch from bc09f71 to 9090635 Compare November 12, 2017 16:17

merge fix

29f0dfd

gafter approved these changes Nov 13, 2017

View reviewed changes

gafter merged commit 5408538 into dotnet:post-dev15.5-contrib Nov 13, 2017

gafter mentioned this pull request Nov 13, 2017

Misleading diagnostic for constant false when clause #18750

Closed

gafter added the Resolution-Fixed The bug has been fixed and/or the requested behavior has been implemented label Nov 13, 2017

sharwell modified the milestones: 15.7, 15.6 Nov 13, 2017

elijah6 deleted the misleadingConstantDiagnostics branch November 14, 2017 19:29

Misleading constant diagnostics #18933

Misleading constant diagnostics #18933

Conversation

elijah6 commented Apr 23, 2017

elijah6 commented Apr 23, 2017

BattleRush commented Apr 23, 2017

elijah6 commented Apr 23, 2017 • edited Loading

elijah6 commented Apr 23, 2017

elijah6 commented Apr 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elijah6 Apr 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented Apr 23, 2017 • edited Loading

sharwell commented May 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharwell commented May 12, 2017

sharwell commented May 18, 2017

elijah6 commented May 18, 2017

gafter commented Sep 25, 2017 • edited Loading

elijah6 commented Oct 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gafter commented Oct 2, 2017

elijah6 commented Nov 13, 2017

gafter commented Nov 13, 2017

elijah6 commented Nov 14, 2017

elijah6 commented Apr 23, 2017 •

edited

Loading

elijah6 Apr 23, 2017 •

edited

Loading

sharwell commented Apr 23, 2017 •

edited

Loading

gafter commented Sep 25, 2017 •

edited

Loading