New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support target-typed new #25196

Merged
merged 38 commits into from Aug 8, 2018

Conversation

@alrz
Copy link
Contributor

alrz commented Mar 2, 2018

@alrz alrz requested a review from dotnet/roslyn-compiler as a code owner Mar 2, 2018

{
// TODO(target-typed-new): Use uncommon data to pass over the already computed
// TODO(target-typed-new): overload resolution results from succeeded conversion
// TODO(target-typed-new): to manually populate a BoundObjectCreationExpression

This comment has been minimized.

@alrz

alrz Mar 2, 2018

Contributor

I held this off until the design is finalized. There is certainly room for improvements i.e. we can avoid some overload resolution pass in some codepathes. I guess we should forbid dynamic arguments for now. When that's confirmed I'll refactor to eliminate redundant analysis and reuse precomputed overload resolution results.

@jcouv jcouv added this to the 16.0 milestone Mar 2, 2018

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Mar 2, 2018

Thanks. Marked as blocked until LDM reviews the language design proposal. #Closed

@jcouv jcouv added the Community label Mar 2, 2018

@alrz alrz force-pushed the alrz:features/target-typed-new branch from 4c1b011 to 54cc99c Mar 2, 2018

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Apr 4, 2018

Marking as "for personal review" for now, so it doesn't show in our "active reviews" query.
LDM is out of session for next 4 weeks or so.
I've added a note for this issue in https://github.com/dotnet/roslyn/projects/27 to remind me to push this onto the agenda once LDM restarts. #Closed

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented May 21, 2018

@alrz Sorry for the delay. LDM discussed this today and supports the feature (as usual, modulo unforseen major issues we might discover later). 🎉

As a result, we can start the review process for the PR. It would probably be good to refresh/rebase as a starting point.

We discussed the overload resolution rules and compat implications (see upcoming notes).
In short, an argument that is a target-typed new behaves like an out var in that it doesn't contribute a type to overload resolution. That behavior mitigates some of the compat implications of the feature.

We didn't get to cover other open issues, but I think we can get them on LDM agenda soon. Let's document them in the championed issue.
#Closed

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 21, 2018

We discussed the overload resolution rules and compat implications (see upcoming notes).
In short, an argument that is a target-typed new behaves like an out var in that it doesn't contribute a type to overload resolution. That behavior mitigates some of the compat implications of the feature.

Interesting! Though somewhat a pity as it means if you have overloads that are otherwise identical you'll have an ambiguity. But that's likely a good place to start from . In the future, it seems like actual rules for deciding how the new call could be part of overload resolution could be added solely to solve those ambiguous issues in a non-breaking fashion.

@Neme12

This comment has been minimized.

Copy link
Contributor

Neme12 commented May 21, 2018

Though somewhat a pity as it means if you have overloads that are otherwise identical you'll have an ambiguity.

Why? There are already perfectly good ways to resolve an ambiguity, the best one being just specifying the type of what you're creating. It's not clear at the call point what it is anyway. Overloading based on the arguments to new() seems really like a bad idea. Overload resolution is already complicated as it is. Why open a whole new can of worms?

Isn't the main scenario for target typed new just:

private readonly Dictionary<(string a, blah b), IEnumerable<Dictionary<string, (int blah, blahlabh b)>))> d = new();

?

I don't see how sprinkling new() all over method calls especially considering how extensively C# uses overload resolution is a good idea or makes the code clearer.

Though somewhat a pity

Can you give an example of code where this would be worth pitying over?

@Neme12

This comment has been minimized.

Copy link
Contributor

Neme12 commented May 21, 2018

I'm sorry, that's just how I see it. Please give me an example of where new() contributing to overload resolution would be useful and not make the code unreadable and error-prone.

I thought this would work just like default - as a shorthand when it's absolutely clear what the target type is. If there's any sort of ambiguity, just specify the type you need. Or if you prefer, use named argument. I don't see any problem with that.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented May 21, 2018

I'm sorry, that's just how I see it. Please give me an example of where new() contributing to overload resolution would be useful and not make the code unreadable and error-prone.

Consider this scenario:

// Util.dll 
public class C1 { } 
public class C2 {
  public C2(int count) { } 
}
public class C3 {
  public static M(C1 p) { } 
  public static M(C2 p) { }
}

Now consider I consume the code from a different library in the following manner:

C3.M(new (1));

So far so good. If new contributed to type inferencee then this would compile fine and everything would be great. Now imagine time passes and the author of Util.dll adds the following constructor:

public C1(int i) { } 

Now suddenly the consume is broken because new (1) is ambiguous between C1 and C2. Expanding this out it essentially means that evyr new constructur is potentially a source breaking change in the face of target typed new. This is the core scenario we're trying to guard against.

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented May 21, 2018

@jaredpar I think you answered a different question, and instead supported @Neme12's concern about ambiguities :-) #Closed

@Neme12

This comment has been minimized.

Copy link
Contributor

Neme12 commented May 21, 2018

@jaredpar Thanks for the response, although I'm a little confused. I was arguing against new contributing to type inference. But I didn't even think of this problem with compatibility. Thanks for the explanation.

I probably didn't make myself clear enough, which is my fault. By "why" I was asking why the proposed behavior is pity and was surprised that changing new to affect overload resolution would be considered a welcome change for a future version.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented May 21, 2018

Thanks for the response, although I'm a little confused. I was arguing against new contributing to type inference

Sorry. I was in a rush to finish my comment before another meeting started and misread your feedback.

But I didn't even think of this problem with compatibility.

My mind is twisted to think about evil things after years of compatibility bugs 😄

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 22, 2018

Why open a whole new can of worms?

I explicitly did not say that. I said precisely that: "that's likely a good place to start from"

:)

I simply said if there was any limitations here they could be considered in the future, not that htey should or that there was any problem here. It was a show of support for this approach precisely because it's likely good enough and because it likely doesn't shut down potential improvements in the future. Both of these are important for me when designing language features. I very much want to ensure that when decisions are made now, they don't shut down possible areas of interest that may arise later.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 22, 2018

I thought this would work just like default - as a shorthand when it's absolutely clear what the target type is. If there's any sort of ambiguity, just specify the type you need. Or if you prefer, use named argument. I don't see any problem with that.

That seems totally fine to me. Importantly, i think that's a great place to start at. And, if it ends up potentially being limiting, is something that could potentially addressed in the future in a way that i do not think would cause breaking changes.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 22, 2018

Now suddenly the consume is broken because new (1) is ambiguous between C1 and C2. Expanding this out it essentially means that evyr new constructur is potentially a source breaking change in the face of target typed new. This is the core scenario we're trying to guard against.

Well, this is a situation with "default" as well. If i have a method:

void M(int i) { }

And someone calls: M(default);

it's now source-breaking if i add:

void M(bool b) { }

because the 'default' is ambiguous.

I think that's an acceptable place to be in.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented May 22, 2018

@CyrusNajmabadi

Well, this is a situation with "default" as well.

The situation is a bit different with default. In the case of default the source breaking change comes when an overload of an existing method is added. That is an understood place where source breaks can happen which default does make worse.

In the case of target typed new the break comes when a constructor is added to a type. This ends up breaking an existing method overload that the type author was quite possible completely unaware of.

The way I think about target typed new is that for the purpose of overload resolution defining a new constructor is like defining a new implicit conversion on the type.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented May 22, 2018

In the case of default the source breaking change comes when an overload of an existing method is added. That is an understood place where source breaks can happen which default does make worse.

I guess i long internalized that adding methods could trivially lead to source-breaks. So having a way for adding constructors to break things doesn't really bother me :)

That said, i'm not pushing for this to be supported. As i said, i think this is a good place to start from. And only at some future point would this need to be revisited if there was particular value in expanding support here.

@jaredpar

This comment has been minimized.

Copy link
Member

jaredpar commented May 22, 2018

@CyrusNajmabadi

I guess i long internalized that adding methods could trivially lead to source-breaks.

Agree. I think the difference here for me is the proximity of the change to the break. When adding an overload I'm both:

  • Aware I'm introducing a potential break. The other overload is generally in the same source file and same section as the new code I'm adding.
  • Able to look at the set of overloads in totality and evaluate the risk of a breaking source change.

Those aren't absolutes: extension methods, partial classes, etc ... can make it harder. But generally speaking true. The same is not true with target type new. The new constructor and the overload it affects don't have to be in the same source file or even the same library.

@alrz alrz force-pushed the alrz:features/target-typed-new branch from 54cc99c to 3dd3a26 May 22, 2018

@jcouv jcouv self-assigned this May 22, 2018

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented May 22, 2018

In short, an argument that is a target-typed new behaves like an out var in that it doesn't contribute a type to overload resolution.

From discussion with @gafter, my summary is incorrect.

Here's a better summary: A target-typed new is never a better "conversion from expression" to one type versus another.

Neal is recommending to wait for LDM to decide on the initializer scenario before moving ahead with this PR.
Here's the example:
M(new () { x = 3 }) where two overloads exist (M(C) and M(D)). Suppose that D doesn't have a property/field named x, does expression new () { x = 3 } convert to D or not?

For the record, some scenarios Neal explained to me:

  • [Updated] If you have Base and Derived, both with empty constructors, then M(new ()) would pick Derived because it is more specific. be ambiguous.
  • M(new (i: 1, j: 2)) when we have two overloads (one with C and the other with D) and two constructors C(int i, int j) and D(int x, int y). Only the overload with C is applicable because expression new (i: 1, j: 2) does not convert to D. #Closed
}
finally
{
this.Reset(ref resetPoint);
this.Release(ref resetPoint);
}

isPossibleArrayCreation =

This comment has been minimized.

@jcouv

jcouv Jul 31, 2018

Member

isPossibleArrayCreation [](start = 12, length = 23)

Just curious: What prompted you to move the initialization of isPossibleArrayCreation outside of the try/finally? #Closed

This comment has been minimized.

@alrz

alrz Aug 1, 2018

Contributor

we capture all the information required in try block, so I thought this would be more clear. #Closed

@jcouv

This comment has been minimized.

Copy link
Member

jcouv commented Jul 31, 2018

        comp.VerifyDiagnostics(

Nice. Thanks! #Closed


Refers to: src/Compilers/CSharp/Test/Emit/CodeGen/CodeGenTupleTest.cs:939 in 141f778. [](commit_id = 141f778, deletion_comment = False)

@@ -9,6 +9,1404 @@ namespace Microsoft.CodeAnalysis.CSharp.UnitTests
{
public partial class IOperationTests : SemanticModelTestBase
{
[CompilerTrait(CompilerFeature.IOperation)]
[Fact]
public void TargetTypedObjectCreationWithMemberInitializers()

This comment has been minimized.

@jcouv

jcouv Jul 31, 2018

Member

Thanks for adding those IOperation tests!
Tagging @333fred @mavasani in case they want to take a look at those tests too. #Closed

OutConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)
IArgumentOperation (ArgumentKind.DefaultValue, Matching Parameter: o) (OperationKind.Argument, Type: null, IsImplicit) (Syntax: '2')
ILiteralOperation (OperationKind.Literal, Type: System.Object, Constant: null, IsImplicit) (Syntax: '2')
InConversion: CommonConversion (Exists: True, IsIdentity: True, IsNumeric: False, IsReference: False, IsUserDefined: False) (MethodSymbol: null)

This comment has been minimized.

@jcouv

jcouv Jul 31, 2018

Member

} [](start = 8, length = 1)

If you don't mind, could you add one IOperations test with arguments in new(...) as well? Thanks

@jcouv
Copy link
Member

jcouv left a comment

Done with review pass (iteration 35).
Only blocking concern is holding onto pooled object (AnalyzedArguments) in the bound tree. Aside from that a couple minor questions/suggestions.
Thanks!

@alrz alrz force-pushed the alrz:features/target-typed-new branch from 6628fda to 85e0a01 Aug 2, 2018

@alrz alrz force-pushed the alrz:features/target-typed-new branch from 85e0a01 to aaa3b35 Aug 2, 2018

@jcouv

jcouv approved these changes Aug 2, 2018

Copy link
Member

jcouv left a comment

LGTM (iteration 37).
Thanks for sorting the AnalyzedArguments situation out.

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Aug 2, 2018

I'm reviewing this.

@@ -1292,7 +1292,14 @@
<!-- BinderOpt is added as a temporary solution for IOperation implementation and should probably be removed in the future -->
<Field Name="BinderOpt" Type="Binder" Null="allow"/>
</Node>


<Node Name="UnboundObjectCreationExpression" Base="BoundExpression">

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

Since the object initializer is logically part of the (un)bound object creation expression, it should be captured here. Probably as an optional InitializerExpressionSyntax. The caller should not assume that the syntax of this bound node is of the proper form, as that would prevent us from using it as an intermediary in translations (of perhaps new, different syntax forms) later.

This comment has been minimized.

@alrz

alrz Aug 8, 2018

Contributor

Done.

Please review the follow-up PR at #29167 after this goes in. thanks.

@@ -128,6 +133,126 @@ internal partial class Binder
{ WasCompilerGenerated = wasCompilerGenerated };
}

private BoundExpression CreateImplicitNewConversion(SyntaxNode syntax, BoundExpression source, Conversion conversion, bool isCast, TypeSymbol destination, DiagnosticBag diagnostics)
{
var node = (ObjectCreationExpressionSyntax)source.Syntax;

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

ObjectCreationExpressionSyntax [](start = 24, length = 30)

As I mentioned in BoundNodes.xml, we should not require a relationship between an UnboundObjectCreationExpression and its syntax node. Please store the initializer (or anything else needed) in the UnboundObjectCreationExpression.

var unboundObjectCreation = (UnboundObjectCreationExpression)source;

// PROTOTYPE(target-typed-new): Reconstruct AnalyzedArguments to avoid binding twice
arguments.Arguments.AddRange(unboundObjectCreation.Arguments);

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

I think this comment can be deleted now?

@checked: false,
explicitCastInCode: isCast,
constantValueOpt: null, // A "target-typed new" would never produce a constant.
type: destination);

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

This should be explicit in the spec, if it isn't already, as the following corresponding code is accepted today:

        const int N = new int();

I think there is a rule that a value type's default constructor can't be used; that is what causes this (commented fact) to be true. #ByDesign

@@ -609,6 +609,9 @@ private static bool IsEncompassingImplicitConversionKind(ConversionKind kind)
case ConversionKind.ImplicitTupleLiteral:
case ConversionKind.ImplicitTuple:
case ConversionKind.ImplicitThrow:

// Added for C# 8.
case ConversionKind.ImplicitNew:
return true;

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

Do we have a test that exercises this branch?

This comment has been minimized.

@alrz

alrz Aug 3, 2018

Contributor

Looks like it's not reachable for ImplicitThrow as well. My understanding is that since both new and throw are convertible to any type, there's no conversion left for these to encompass. Is that correct?


case ConversionKind.ImplicitNew:
return rewrittenOperand;

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

Is this branch exercised by tests? I'm wondering if it is reachable.

This comment has been minimized.

@alrz

alrz Aug 3, 2018

Contributor

Now that we directly return the bound object creation, it's not reachable. will revert.

);
}
}
}

This comment has been minimized.

@gafter

gafter Aug 3, 2018

Member

Can you please add tests to show how the semantic model works? In normal (non-error) cases, e.g. for the whole expression and its argument expressions, but also inside the object initializer, and inside the object initializer when there is no target type (error scenario), for example

    M(new() { X = <bind>N()</bind> }); // M not found, but N is found. Also try when M is found but no X is found

How does GetTypeInfo work on the subexpression N()? Even if it doesn't work, it would be nice to have a test documenting the current (expected?) behavior.

This comment has been minimized.

@alrz

alrz Aug 4, 2018

Contributor

If it's ok I'll address these in the follow-up PR.

@gafter

This comment has been minimized.

Copy link
Member

gafter commented Aug 3, 2018

Done with review pass (Iteration 37). Generally it is looking quite good, except as noted.


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

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Aug 8, 2018

The next PR based on these changes is opened at #29167

@gafter

gafter approved these changes Aug 8, 2018

Copy link
Member

gafter left a comment

:shipit:

@gafter gafter merged commit 1e246ca into dotnet:features/target-typed-new Aug 8, 2018

14 of 16 checks passed

windows_debug_spanish_unit32_prtest Started.
Details
windows_debug_vs-integration_prtest Started.
Details
WIP ready for review
Details
license/cla All CLA requirements met.
Details
microbuild_prtest Build finished.
Details
ubuntu_16_debug_prtest Build finished.
Details
ubuntu_16_mono_debug_prtest Build finished.
Details
windows_build_correctness_prtest Build finished.
Details
windows_coreclr_debug_prtest Build finished.
Details
windows_coreclr_release_prtest Build finished.
Details
windows_debug_unit32_prtest Build finished.
Details
windows_debug_unit64_prtest Build finished.
Details
windows_determinism_prtest Build finished.
Details
windows_release_unit32_prtest Build finished.
Details
windows_release_unit64_prtest Build finished.
Details
windows_release_vs-integration_prtest Build finished.
Details

@alrz alrz deleted the alrz:features/target-typed-new branch Aug 8, 2018

@alrz alrz referenced this pull request Nov 8, 2018

Open

Update target-typed-new.md #1989

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment