Fix conversions on default literals by jcouv · Pull Request #37596 · dotnet/roslyn

jcouv · 2019-07-30T22:40:07Z

This PR fixes two problems with default literal (from C# 7.1):

from a language perspective, we should not have re-used the "null literal conversion" (which is specifically a conversion from the null literal to a nullable value type), so I'm introducing a "default literal conversion" (conversion from the default literal to any type)
from a binding perspective, whenever a default was getting converted, we would incorrectly give it a natural type. This PR follows Neal's recent PRs by having a BoundDefaultExpression and a BoundConvertedDefaultExpression. This affects some scenarios
1. default as TClass: becomes an error
2. $"{default}": use object as target-type for expressions that don't have a natural type
3. using (default) ...: becomes an error
  Also, during return type inference of lambdas, expressions are neither required to be target-typed or to have a natural type.
x == default will no longer bind as object equality.

But I reverted the part of this PR which affected public APIs (.Type in particular) because it had significant effect on the IDE (and therefore likely other consumers).

Fixes #31029
Fixes #18609
Fixes #35684 and #36492 (incorrectly emitting default as null, missing error, fix is a breaking change)
Fixes #30384

Relates to #22125

Opened issue tracking spec changes: #37821

Update: this breaking change was later adjusted in 16.4p2.

jcouv · 2019-08-08T17:40:58Z

I'm investigating some test failures. Marking PR as "personal" for now. #Closed

jcouv · 2019-08-08T19:34:24Z

src/Workspaces/Core/Portable/FlowAnalysis/CustomDataFlowAnalysis.cs

@mavasani for review of the change to CustomDataFlowAnalysis.cs, DataFlowAnalyzer.cs and BasicBlockReachabilityDataFlowAnalyzer.cs.
For background, the comparison of default to an unconstrained type was improperly allowed (and interpreted as a comparison against null). With this PR, this becomes an error. #Resolved

Hum, I just realized my fix is wrong :-( I'll take another look #Closed

Yes, the intent is to allow the type parameter to be either a struct or class, and if it is default value then initialize it by invoking the GetEmptyAnalysisData API. What would be the recommended alternative here? #Resolved

@mavasani The way I fixed this is to require GetCurrentAnalysisData to return a value (possibly an empty one).

In reply to: 314401113 [](ancestors = 314401113)

I see that you updated the comment on the abstract method GetCurrentAnalysisData, but did not change the override which may return null.

roslyn/src/Workspaces/Core/Portable/FlowAnalysis/SymbolUsageAnalysis/SymbolUsageAnalysis.DataFlowAnalyzer.cs

Lines 149 to 150 in 7eb2327

public override BasicBlockAnalysisData GetCurrentAnalysisData(BasicBlock basicBlock)

=> _analysisData.GetBlockAnalysisData(basicBlock);

Can you change the override to _analysisData.GetBlockAnalysisData(basicBlock) ?? GetEmptyAnalysisData();? #Resolved

alrz · 2019-08-09T07:13:47Z

after this goes in I'll try to sync target-typed-new branch with 16.4p1. let me know if there's other considerations. thanks. #Closed

jcouv · 2019-08-09T16:48:50Z

@alrz Sounds good. Thanks! #Closed

RikkiGibson

I read the diff from starting my last review to now. LGTM.

jcouv · 2019-08-20T23:57:32Z

I've rebased to resolve conflict.

jcouv · 2019-08-21T23:10:58Z

@mavasani Did you have any other feedback? Thanks

mavasani · 2019-08-22T17:57:44Z

src/EditorFeatures/CSharpTest/ReplaceDefaultLiteral/ReplaceDefaultLiteralTests.cs

Is the input source valid code now, hence CSharpReplaceDefaultLiteralCodeFixProvider is not triggered?

No, case default: is still illegal. The behavior change here is a casualty of binding change: the compiler no longer tries to give default a type via conversion, so the IDE doesn't get a type to to drive the refactoring.

I took another look and managed to tweak that, which allowed to revert most of the IDE test changes. Thanks!

mavasani

Thanks, IDE changes LGTM!

RikkiGibson · 2019-08-23T22:47:49Z

src/ExpressionEvaluator/CSharp/Test/ExpressionCompiler/ExpressionCompilerTests.cs

                testData = new CompilationTestData();
                context.CompileExpression("default", DkmEvaluationFlags.None, ImmutableArray<Alias>.Empty, out error, testData);
-                Assert.Equal(error, "error CS8716: No target type was found for the default literal.");
+                Assert.Equal(error, "error CS8716: No target type was found found for the default literal.");


This is really what the test wants? Strange..

dotnet/roslyn#37596 includes a breaking change in the 5.0 branch. This function is intended to ensure reads happen from the array, so the test isn't strictly necessary. Fixes dotnet#865

dotnet/roslyn#37596 includes a breaking change in the 5.0 branch. This function is intended to ensure reads happen from the array, so the test isn't strictly necessary. Fixes #865

Since updating to Microsoft.CodeAnalysis 3.3.1, the default keyword is resolved to a vaule instead of the a default expression. Probably comes from dotnet/roslyn#37596

* Update to Microsoft.CodeAnalysis 3.3.1 This updates to the latest Microsoft.CodeAnalysis packages for C#8 support. Fixes the following error on a project that uses nullable reference types: Microsoft.DocAsCode.Exceptions.DocfxException: Unable to generate spec reference for !: ---> System.IO.InvalidDataException: Fail to parse id for symbol in namespace . at Microsoft.DocAsCode.Metadata.ManagedReference.YamlModelGenerator.AddSpecReference (Microsoft.CodeAnalysis.ISymbol symbol, System.Collections.Generic.IReadOnlyList`1[T] typeGenericParameters, System.Collections.Generic.IReadOnlyList`1[T] methodGenericParameters, System.Collections.Generic.Dictionary`2[TKey,TValue] references, Microsoft.DocAsCode.Metadata.ManagedReference.SymbolVisitorAdapter adapter) [0x0005e] in <27b606b838ab45b788854d5e45efa610>:0 at Microsoft.DocAsCode.Metadata.ManagedReference.SymbolVisitorAdapter.AddSpecReference (Microsoft.CodeAnalysis.ISymbol symbol, System.Collections.Generic.IReadOnlyList`1[T] typeGenericParameters, System.Collections.Generic.IReadOnlyList`1[T] methodGenericParameters) [0x00000] in <27b606b838ab45b788854d5e45efa610>:0 * Fix TODO for space after tuple Since updating Microsoft.CodeAnalysis to 3.3.1, the automatic formatting for tuples in C# has been fixed, so update tests accordingly. dotnet/roslyn#29405 * Fix TestCSharpFeature_Default_7_1Class Since updating to Microsoft.CodeAnalysis 3.3.1, the default keyword is resolved to a vaule instead of the a default expression. Probably comes from dotnet/roslyn#37596

jcouv added the Area-Compilers label Jul 30, 2019

jcouv added this to the 16.4 milestone Jul 30, 2019

jcouv self-assigned this Jul 30, 2019

jcouv force-pushed the default-conv branch 3 times, most recently from d039e5d to 28a2b18 Compare July 30, 2019 23:59

This was referenced Aug 7, 2019

null literal should get a ConvertedType #37798

Open

Spec: record details of target-typing #37821

Open

jcouv force-pushed the default-conv branch 2 times, most recently from f1306f6 to 3176689 Compare August 8, 2019 00:51

jcouv marked this pull request as ready for review August 8, 2019 00:53

jcouv requested a review from a team as a code owner August 8, 2019 00:53

jcouv force-pushed the default-conv branch from 9710a5c to c610a38 Compare August 8, 2019 01:08

jcouv added the PR For Personal Review Only The PR doesn’t require anyone other than the developer to review it. label Aug 8, 2019

jcouv force-pushed the default-conv branch from c610a38 to 2c354cd Compare August 8, 2019 18:19

jcouv requested review from a team as code owners August 8, 2019 18:19

jcouv force-pushed the default-conv branch 2 times, most recently from c9c8419 to b498181 Compare August 8, 2019 19:22

jcouv commented Aug 8, 2019

View reviewed changes

jcouv force-pushed the default-conv branch from c6c5585 to 2c547f5 Compare August 9, 2019 00:13

jasonmalinowski removed request for a team August 9, 2019 00:13

jcouv force-pushed the default-conv branch from 2c547f5 to 6695124 Compare August 9, 2019 01:02

jcouv force-pushed the default-conv branch 2 times, most recently from 0f8b9da to 9980615 Compare August 9, 2019 16:48

RikkiGibson approved these changes Aug 20, 2019

View reviewed changes

jcouv force-pushed the default-conv branch from 1bab8b1 to 0d6e5ce Compare August 20, 2019 23:57

mavasani reviewed Aug 22, 2019

View reviewed changes

jcouv force-pushed the default-conv branch from 5e1dd7c to eabea9d Compare August 22, 2019 22:17

mavasani approved these changes Aug 22, 2019

View reviewed changes

jcouv force-pushed the default-conv branch from eabea9d to ce60ed1 Compare August 22, 2019 23:34

Fix conversions on default literals

682ce10

jcouv force-pushed the default-conv branch from 0a6eabe to 682ce10 Compare August 23, 2019 16:50

jcouv and others added 2 commits August 23, 2019 11:23

Fix bad edit

2c24d03

Update a couple of tests

61c2452

jcouv force-pushed the default-conv branch from f8f3cf8 to 61c2452 Compare August 23, 2019 18:53

jcouv changed the base branch from release/dev16.4-preview1 to master August 23, 2019 19:00

Fix EE test baseline with new diagnostic text

0e671a7

RikkiGibson reviewed Aug 23, 2019

View reviewed changes

jcouv added 2 commits August 24, 2019 10:54

Fix typo

9fcf26b

Fix EE test baseline

5d7d0bb

jcouv merged commit 1f867ac into dotnet:master Aug 25, 2019

jcouv deleted the default-conv branch August 25, 2019 05:55

jcouv modified the milestones: 16.4, 16.4.P1 Sep 6, 2019

billwert mentioned this pull request Sep 9, 2019

Fix lab break dotnet/performance#867

Merged

jcouv mentioned this pull request Sep 25, 2019

x == default is valid object equality for reference types #38853

Merged

jcouv mentioned this pull request Jan 8, 2020

Give special diagnostic for t == default #40801

Merged

	public override BasicBlockAnalysisData GetCurrentAnalysisData(BasicBlock basicBlock)
	=> _analysisData.GetBlockAnalysisData(basicBlock);

Conversation

jcouv commented Jul 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Aug 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv Aug 8, 2019 • 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!

jcouv Aug 8, 2019 • 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!

mavasani Aug 15, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 19, 2019

Choose a reason for hiding this comment

Uh oh!

mavasani Aug 22, 2019 • 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!

alrz commented Aug 9, 2019 • edited by jcouv Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcouv commented Aug 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RikkiGibson left a comment

Choose a reason for hiding this comment

Uh oh!

jcouv commented Aug 20, 2019

Uh oh!

jcouv commented Aug 21, 2019

Uh oh!

mavasani Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

jcouv Aug 22, 2019

Choose a reason for hiding this comment

Uh oh!

mavasani left a comment

Choose a reason for hiding this comment

Uh oh!

RikkiGibson Aug 23, 2019

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcouv commented Jul 30, 2019 •

edited

Loading

jcouv commented Aug 8, 2019 •

edited

Loading

jcouv Aug 8, 2019 •

edited

Loading

jcouv Aug 8, 2019 •

edited

Loading

mavasani Aug 15, 2019 •

edited by jcouv

Loading

mavasani Aug 22, 2019 •

edited

Loading

alrz commented Aug 9, 2019 •

edited by jcouv

Loading

jcouv commented Aug 9, 2019 •

edited

Loading