UseDefaultLiteral and RemoveUnnecessaryCast shouldn't introduce invalid default literal in pattern or case label by Neme12 · Pull Request #25538 · dotnet/roslyn

Neme12 · 2018-03-16T20:42:06Z

Customer scenario

Type case default(bool):, or case (bool)default:, or b is default(bool), or b is (bool)default.
In all those cases, no code fix should be offered to simplify to the default literal, as this results in a compilation error.

Bugs this fixes

fixes #25456

Risk

Performance impact

Low. A few syntax checks were added to the logic that verifies if a change results in different semantics.

Is this a regression from a previous update?

No

Root cause analysis

At least for the is case, it was allowed for a short while, then we made a language change to reserve default for use in C# 8 match expressions. For case, I don't remember.

How was the bug found?

Reported internally.

…rsion

Neme12 · 2018-03-16T20:43:18Z

+            // First, check to see if the node ultimately parenting this cast has any syntax errors.
+            // If so, we bail. If removing the cast introduces a syntax error, bail out too.
+            if (speculationAnalyzer.SemanticRootOfOriginalExpression.ContainsDiagnostics ||
+                speculationAnalyzer.SemanticRootOfReplacedExpression.ContainsDiagnostics)


~~@CyrusNajmabadi what do you think about this approach? are there any concerns with this? performance concerns possibly?~~

~~this seems like generally a good idea but could also hide bugs if for some reason we generate an invalid syntax node~~

whatever we do (whtether a check for diagnostics or special-case check for default inside a switch case/pattern) should probably be done inside the speculation analyzer itself in ReplacementChangesSemantics - there are probably other IDE features that might introduce broken code here and doing that would ensure they all behave correctly and don't need to each handle this separately

another thing i like about this is that it doesn't duplicate the logic in the compiler

This seems reasonable to me.

@CyrusNajmabadi well it shouldn't 😄 that's all wrong, forget what i said.

the "replaced" expression does not contain any new diagnostics... all the tests passed because i didn't pass in c# 7.1 to the tests that expect to not get this offered - it simply wasn't offered because it used c# 7.0 where default is always a syntax error

turns out manual testing is still useful

Neme12 · 2018-03-16T20:49:17Z

            }

+            if (defaultExpression.WalkUpParentheses().IsParentKind(SyntaxKind.ConstantPattern, SyntaxKind.CaseSwitchLabel))
+            {


if it's done in the speculation analyzer, this can be removed

Neme12 · 2018-03-16T20:58:35Z

+    {
+        switch (true)
+        {
+            case ((bool)default):


obviously another bug - it adds unnecessary parentheses

created an issue here: #25554

Neme12 · 2018-03-16T21:00:55Z

        protected abstract TestWorkspace CreateWorkspaceFromFile(string initialMarkup, TestParameters parameters);

+        private TestParameters WithRegularOptions(TestParameters parameters)
+            => parameters.WithParseOptions(parameters.parseOptions?.WithKind(SourceCodeKind.Regular));


this is an important change so that TestMissingInRegularAndScriptAsync and the other methods actually use the language version we asked for by giving ParseOptions inside TestParameters, rather than ignoring it

did this reveal any errant tests that were passing when tehy shouldn't hve been?

actually no, i was surprised by that

but it revealed a lot of tests that would have passed even if the code they were supposed to test didn't work

Neme12 · 2018-03-17T00:36:29Z

+                    // We can't have a default literal inside a case label or constant pattern.
+                    return true;
+                }
+            }


@CyrusNajmabadi having the check here will be the most correct and will work for all IDE features equally well, the only thing I don't like is that this is just a special edge case that is at the very beginning and steals focus from the rest of this method which is far more important

also, this doesn't really feel like a "changing the semantics" all that much

But I really feel like this code has to be somewhere in the speculation analyzer and not scattered around individual features. Doing this fixes bugs in other features too. One example I found is:

const bool b = default; switch (true) { case b: break; }

do inline temporary on 'b' - it will use replace 'b' with 'default' and produce the error.
But with this change, it will introduce a cast and the resulting code will be correct:

switch (true) { case ((bool)(default)): break; }

edit: I'm wrong, this particular case actually works with either approach because the simplifier uses the same helper.

also, this doesn't really feel like a "changing the semantics" all that much

"changes semantics" is just the term we used for "breaks the code" :)

the only thing I don't like is that this is just a special edge case that is at the very beginning and steals focus from the rest of this method which is far more important

The method is just a bunch of checks that was added in the order we found and fixed thigns while doing this feature. I don't see any problem with your addition.

Neme12 · 2018-03-17T00:44:06Z

    {
-        private static ITypeSymbol GetOuterCastType(ExpressionSyntax expression, SemanticModel semanticModel, out bool parentIsOrAsExpression)
+        private static ITypeSymbol GetOuterCastType(ExpressionSyntax expression, SemanticModel semanticModel,
+            out bool parentIsIsOrAsExpression)


maybe this name doesn't roll off the tongue as well, but at least it makes sense - i couldn't figure out what the old one was supposed to mean until i looked at the implementation

jcouv

LGTM Thanks

jcouv · 2018-03-21T05:44:14Z

@dotnet/roslyn-ide Please review this community PR. Thanks

jinujoseph · 2018-04-01T15:46:10Z

@dotnet-bot Test this please

jinujoseph · 2018-04-01T16:31:20Z

@dotnet-bot retest this please

Neme12 · 2018-04-01T16:50:56Z

@dotnet-bot retest windows_debug_unit32_prtest please

Neme12 · 2018-04-03T22:01:18Z

@jinujoseph You were right to try to retest this, there was the build issue with a missing using again. Thanks

Neme12 added 5 commits March 16, 2018 18:28

Fix for tests: TestInRegularAndScriptAsync should respect language ve…

ec12612

…rsion

Disabling UseDefaultLiteral in a switch case label

4abaee1

Disabling inside constant pattern

cf12333

Adding tests inside 'is'

4b80370

RemoveUnnecessaryCast shouldn't introduce the error either

b745064

Neme12 requested a review from a team as a code owner March 16, 2018 20:42

Neme12 commented Mar 16, 2018

View reviewed changes

Neme12 added 3 commits March 16, 2018 22:59

Using C# 7.1 in all relevant tests

4480314

Fixing the check in IsUnnecessaryCast

7ed86f3

Moving the logic to SpeculationAnalyzer

2726b77

Neme12 commented Mar 17, 2018

View reviewed changes

Adding issue links

0af8039

Neme12 mentioned this pull request Mar 17, 2018

"Simplify Default Expression" produces bad constant pattern. #25456

Closed

jcouv added Area-IDE Community The pull request was submitted by a contributor who is not a Microsoft employee. labels Mar 17, 2018

jcouv approved these changes Mar 17, 2018

View reviewed changes

jcouv added this to the 15.8 milestone Mar 17, 2018

Neme12 mentioned this pull request Mar 17, 2018

'Remove unnecessary cast' adds unnecessary parentheses #25554

Closed

jinujoseph approved these changes Apr 1, 2018

View reviewed changes

Neme12 closed this Apr 1, 2018

Neme12 reopened this Apr 1, 2018

Merge branch 'master' into useDefaultLiteral

c8468ec

Adding missing using after merge

93a4858

jinujoseph merged commit 81c63b7 into dotnet:master Apr 4, 2018

Neme12 deleted the useDefaultLiteral branch April 6, 2018 06:52

Conversation

Neme12 commented Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Customer scenario

Bugs this fixes

Risk

Performance impact

Is this a regression from a previous update?

Root cause analysis

How was the bug found?

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Neme12 Mar 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Mar 21, 2018

Uh oh!

jinujoseph commented Apr 1, 2018

Uh oh!

jinujoseph commented Apr 1, 2018

Uh oh!

Neme12 commented Apr 1, 2018

Uh oh!

Neme12 commented Apr 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Neme12 commented Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 16, 2018 •

edited

Loading

Neme12 Mar 17, 2018 •

edited

Loading

Neme12 Mar 17, 2018 •

edited

Loading

Neme12 commented Apr 3, 2018 •

edited

Loading