Rebase target-typed-new branch on top of master #39658

alrz · 2019-11-03T20:42:29Z

Relates to #28489 (test plan for "target-typed new")

jcouv · 2019-12-11T00:26:47Z

@alrz Thanks for refreshing the target-typed new branch. Let me know when this is ready for a look. #Resolved

alrz · 2019-12-13T19:18:58Z

The PR is almost ready - most of the things just work due to recent changes for handling target-typed expressions, Tuple equality still lacks in some scenarios (see TestTupleEquality01). Other than that I guess I'd like to get feedback on possibly new conventions that we need to follow. Thanks. #Closed

jcouv · 2019-12-13T19:34:36Z

Ok :-)
I'll take a look early next week. Thanks! #Resolved

jcouv · 2019-12-17T19:07:21Z

Looking at this draft PR now.
This PR should probably not target master. Maybe we should review as-is, then we'll retarget to the feature branch once sign-offs are completed? #Resolved

src/Compilers/CSharp/Portable/CSharpResources.Designer.cs

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

src/Compilers/CSharp/Test/Syntax/Parsing/ParsingTests.cs

src/Compilers/CSharp/Test/Syntax/Parsing/TargetTypedObjectCreationParsingTests.cs

jcouv · 2019-12-17T19:38:06Z

src/Compilers/CSharp/Portable/BoundTree/Formatting.cs

+
+ internal partial class UnboundObjectCreationExpression
+ {
+ public override object Display => "new()";


"new()"; [](start = 42, length = 8)

~~nit: should we display the arguments too? or maybe put an ellipsis new(...)~~
Update: I see this is used for displaying diagnostics #Closed

We have three options:

always new() (current iteration)

always new(...) (previous iteration)

conditionally new() if there's no args, otherwise new(...)

I didn't like (2) because I suspect we'll have new() most of the time. I'm fine with other two, I went with (1) for now to keep things simple.

#Closed

I think it's fine to leave as-is (option 1). Thanks #Closed

Given that this is used in error output, I don't think that this is fine. We should do option 3, and correctly display the arguments. #Closed

@alrz I didn't see any response to this? #Closed

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

src/Compilers/CSharp/Portable/FlowAnalysis/AbstractFlowPass.cs

src/Compilers/CSharp/Portable/FlowAnalysis/NullableWalker.cs

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

src/Compilers/CSharp/Portable/Symbols/Source/ParameterHelpers.cs

jcouv · 2019-12-17T20:40:19Z

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

+ // We manually create an ImplicitNullable conversion
+ // if the destination is nullable, in which case we
+ // target the underlying type e.g. `S? x = new();`
+ // is actually identical to `S? x = new S();`.


is actually identical to S? x = new S();. [](start = 19, length = 43)

📝 let's remember to capture that in speclet #Closed

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

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

jcouv · 2019-12-17T20:52:49Z

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

@@ -649,8 +649,8 @@ private BoundExpression BindSimpleBinaryOperator(BinaryExpressionSyntax node, Di
 {
 resultSignature = signature;
 HashSet<DiagnosticInfo> useSiteDiagnostics = null;
- bool leftDefault = left.IsLiteralDefault();
- bool rightDefault = right.IsLiteralDefault();
+ bool leftDefault = left.IsLiteralDefaultOrTypelessNew();


left.IsLiteralDefaultOrTypelessNew(); [](start = 39, length = 37)

📝 let's make sure that the speclet captures the rules around new() and comparisons (when is object equality valid?)

# Conflicts: # src/Compilers/CSharp/Portable/CSharpResources.Designer.cs # src/Compilers/CSharp/Portable/CSharpResources.resx

alrz · 2020-02-20T15:34:31Z

We should add a flag to IObjectCreationOperation, and conditionally include it in the writer when true.

I did not go though IOperation changes because it would need public API review and doesn't help much to be added in this PR. I see it's already tracked in the test plan (#28489)

edit: also added a reminder for speculative semantic model

333fred · 2020-02-25T18:50:49Z

src/Compilers/CSharp/Portable/BoundTree/BoundObjectCreationExpression.cs

 { }

 public BoundObjectCreationExpression Update(MethodSymbol constructor, ImmutableArray<BoundExpression> arguments, ImmutableArray<string> argumentNamesOpt, ImmutableArray<RefKind> argumentRefKindsOpt, bool expanded,
 ImmutableArray<int> argsToParamsOpt, ConstantValue constantValueOpt, BoundObjectInitializerExpressionBase initializerExpressionOpt, Binder binderOpt, TypeSymbol type)
 {
- return this.Update(constructor, ImmutableArray<MethodSymbol>.Empty, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, argsToParamsOpt, constantValueOpt, initializerExpressionOpt, binderOpt, type);
+ return this.Update(constructor, ImmutableArray<MethodSymbol>.Empty, arguments, argumentNamesOpt, argumentRefKindsOpt, expanded, argsToParamsOpt, constantValueOpt, initializerExpressionOpt, wasTargetTyped: false, binderOpt, type);


wasTargetTyped: false, [](start = 201, length = 22)

These should be using the current state of wasTargetTyped, not setting an explicit false.

333fred

LGTM (commit 41), with one more suggestion above.

# Conflicts: # src/Compilers/CSharp/Portable/Errors/MessageID.cs # src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

alrz · 2020-02-26T22:24:00Z

@jcouv @333fred I think I've addressed all the comments.

jcouv · 2020-02-26T23:34:58Z

Yay!
Steps forward:

I'll reset the features/target-typed-new branch to match master
retarget this PR to the feature branch
merge this PR

After that, we can discuss and address the remaining test plan items (semantic model, IOperation, IDE stuff, manual validation)

jcouv · 2020-02-26T23:45:04Z

Steps 1 and 2 are done.
Will squash the PR when CI is green.

jcouv · 2020-02-27T00:02:43Z

src/Compilers/CSharp/Portable/BoundTree/BoundNodes.xml

@@ -1542,6 +1542,19 @@
 <Field Name="ResultKind" PropertyOverrides="true" Type="LookupResultKind"/>
 </Node>

+ <!--
+ This node is used to represent a target-typed object creation expression
+ It does not survive past initial binding.


nit: This only survives past initial binding in error cases

actually it doesn't (due to BindToNaturalType usages), unless we visit unconverted nodes (like in NullableWalker)

jcouv · 2020-02-27T00:04:47Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

@@ -5890,6 +5890,7 @@ private enum ParseTypeMode
 AfterIs,
 DefinitePattern,
 AfterOut,
+ AfterRef,


I don't understand why we're seeing this change in the diff. I would expect this diff (at least in github UI) to only show the target-typed-new changes, but nothing from other merges.
Am I missing something?

This is added in this PR, to avoid cascading diagnostics e.g. for new ref

jcouv · 2020-02-27T00:06:05Z

src/Compilers/CSharp/Portable/PublicAPI.Unshipped.txt

@@ -8,6 +8,12 @@
 *REMOVED*static Microsoft.CodeAnalysis.CSharp.CSharpSyntaxTree.ParseText(string text, Microsoft.CodeAnalysis.CSharp.CSharpParseOptions options = null, string path = "", System.Text.Encoding encoding = null, System.Collections.Immutable.ImmutableDictionary<string, Microsoft.CodeAnalysis.ReportDiagnostic> diagnosticOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.SyntaxTree
 *REMOVED*static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParseSyntaxTree(Microsoft.CodeAnalysis.Text.SourceText text, Microsoft.CodeAnalysis.ParseOptions options = null, string path = "", System.Collections.Immutable.ImmutableDictionary<string, Microsoft.CodeAnalysis.ReportDiagnostic> diagnosticOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.SyntaxTree
 *REMOVED*static Microsoft.CodeAnalysis.CSharp.SyntaxFactory.ParseSyntaxTree(string text, Microsoft.CodeAnalysis.ParseOptions options = null, string path = "", System.Text.Encoding encoding = null, System.Collections.Immutable.ImmutableDictionary<string, Microsoft.CodeAnalysis.ReportDiagnostic> diagnosticOptions = null, System.Threading.CancellationToken cancellationToken = default(System.Threading.CancellationToken)) -> Microsoft.CodeAnalysis.SyntaxTree
+*REMOVED*Microsoft.CodeAnalysis.CSharp.Syntax.ObjectCreationExpressionSyntax.ArgumentList.get -> Microsoft.CodeAnalysis.CSharp.Syntax.ArgumentListSyntax


I would only expect added APIs, but no removed APIs.

This has turned to an override now.

alrz · 2020-02-27T08:39:49Z

src/Compilers/CSharp/Portable/Parser/LanguageParser.cs

- else
+
+ var name = this.CreateMissingIdentifierName();
+ if (mode != ParseTypeMode.NewExpression)


This is where we want to distinguish between NewExpression and AfterRef

alrz · 2020-02-27T11:19:15Z

I think it's worth considering interaction with new[] while we reconfirm with LDM
We may want to change conversion rules based on that. See dotnet/csharplang#2701 (comment)

jcouv · 2020-03-06T21:34:11Z

Merged!
Thanks @alrz for all the work and dedication. It's a pretty cool feature :-)

Next steps:

I think there were a few more test scenarios suggested above (speculative semantic model and maybe some other things), could you handle those? (@333fred could you recap what you had identified?)
I'll raise in LDM again as final greenlight before merging to master
I'll do an IDE test pass on the feature and implement FAR.
We should refresh the speclet and make it more spec-like (ie. identify the areas of the specification that need updating)

alrz · 2020-03-07T17:58:37Z

I think there were a few more test scenarios suggested above (speculative semantic model and maybe some other things)

For TypeInfo, we never attempt to convert a typeless expression to get a type. The result is as good as default literals: no type.

We should refresh the speclet

There's dotnet/csharplang#1989 though it's out of sync.
I think we should also mention object equality (probably should be an error)

333fred · 2020-03-09T16:53:42Z

My list is:

IOperation changes. These don't require an explicit api design meeting ahead of time, they're very small. We'll take the change and do it as part of the general API review session for whatever VS release this ships in preview with.
SpeculativeModel changes. We don't need to try and infer anything for GetSpeculativeTypeInfo, but when we provide an entire statement (TryGetSpeculativeSemanticModel) that should end up inferring type info.

alrz added 4 commits November 4, 2019 00:10

Rebase target-typed-new branch on top of master

e3346a7

Fix formatting

c91b8a9

Move method

6e801fe

Adjust baseline

1e02465

jinujoseph added Area-Infrastructure Area-Compilers Community The pull request was submitted by a contributor who is not a Microsoft employee. and removed Area-Infrastructure labels Nov 5, 2019