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

Test plan for "target-typed new" #28489

Open
jcouv opened this Issue Jul 12, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@jcouv
Copy link
Member

jcouv commented Jul 12, 2018

Compiler (general test plan for reference):

  • LangVersion
  • overload resolution:
    • ...
  • arglist
  • in local declaration for nullable type (TestNullableType1), or in invocation as parameter of nullable type (TestNullableType2)
  • as default parameter value (TestParameterDefaultValue), in params (TestParams)
  • in attribute argument (should fail depending on the type)
  • struct with a 1-constructor, but no 0-constructor
  • in string interpolation
  • in object initializer (new C() { x = new() }), collection initializer (new C() { new D(1), new D(2), new() })
  • with explicit cast (TestBadTargetType_Cast)
  • is it possible to have a constant value? (no)
  • returning in lambda (with single or multiple returns), in async lambda
  • returning in ref-returning method
  • with unary operators (+new(), ...), equality (1 == new()), compound (x += new()), ternary (x ? y : new() and vice-versa)
  • in using, in nameof(), in typeof(), in sizeof(), in foreach, in deconstruction (var (x, y) = new();), in switch, in patterns, in if, in while, in fixed, as an expression-statement (new();), in lock
  • as initial value for an auto-property
  • returning in async method, returning from IEnumerable-returning method, yielding from iterator method
  • assign to var (TestBadTargetType_Assignment), to nullable type (TestNullableType1),
  • semantic model (no natural type, gets a converted type)
  • IOperation
  • new() { 42 }
  • test with enum type, array type, pointer type (bad), test in anonymous type (bad),
  • ref local
  • use as L-value (new() = 1;)
  • test with collection initializer (new() { 1, 2, 3 })
  • update the compiler test plan
  • update the speclet
    -[ ] test interaction with any C# 8.0 feature that was merged earlier (??=, async-streams, nullability, Ranges, so far)

IDE:

  • formatting
    • no space in new() (SpacingInTargetTypedNew)
    • auto-formatting when typing (string a, string b)[] ab = new (string a, string b)[1]; vs. C c = new();
  • FindAllReferences
  • QuickInfo should show the type on new
  • UpgradeProject (typically just works)
  • add/extend UseImplicitType and UseExplicitType refactorings on object creation
  • colorization
  • SignatureHelp should work (it should help tying object creation arguments)
  • Is the type of new() visible by hovering, when debugging?
  • GoTo on new() should go to the proper constructor
  • test existing fixers UseImplicitType and UseExplicitType (for example, List<string> list = new(); should never offer to make the declaration var)
  • code style (prefer implicit/explicit)

Initial PR: #25196
Proposal: https://github.com/dotnet/csharplang/blob/master/proposals/target-typed-new.md
Championed issue (with LDM history): dotnet/csharplang#100

@jcouv jcouv added this to the 16.0 milestone Jul 12, 2018

@jcouv jcouv self-assigned this Jul 12, 2018

@jcouv jcouv added this to Requires runtime or BCL work in Compiler: Julien's umbrellas Jul 29, 2018

@jcouv jcouv moved this from Requires runtime or BCL work to Reference in Compiler: Julien's umbrellas Jul 29, 2018

@jcouv jcouv removed this from Reference in Compiler: Julien's umbrellas Jul 29, 2018

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Aug 8, 2018

@jcouv What is the intended behavior for operators? My guess is that for equality, it should behave exactly the same as new T() == x where T is inferred from the type of the other operand (if any). unary operators would be all disallowed.

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Aug 8, 2018

class D {}

class C
{
    public static bool operator+(C s, D c);
    public static bool operator+(C s, C c);
    static void Main()
    {
        var x =  new C() + new(); // error? what if we remove either or both of operators above?
    }
}

default produces an error, which I think is the most sensible output here as well.

Note: if we allow this we probably shouldn't infer string or object from string concatenation.

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Aug 8, 2018

I agree. The behavior for default is a good guide.

Looking at the default proposal, I see some other interesting cases (annotated with my expectations, let me know if they make sense):

  • new() is T (disallow)
  • x is new() (disallow since not constant)
  • new() as RefType (allow)
  • throw new(); (disallow since no type and not null literal)
  • new() == new() (disallow since no target type)
  • new() == <expr> (allow as long as <expr> has a type)
  • new() == (1, 2) (kinda useless, probably should fail because new() isn't allowed to take tuple type, looks like LDM allowed it after all)
  • case new(): (disallow since not constant)

As a side note, I've updated the proposal for default to list LDM meeting notes, if that helps.

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Aug 8, 2018

Do we want to only permit equality (user-defined and built-in) among comparison and arithmetic operators?

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Aug 8, 2018

I don't see a reason to restrict operators, as long as we have a type to target and that type is allowed for target-typed new.

This rules out unary operators (no type to target).

For types that are disallowed, it would be good to update the proposal to match LDM notes.
If I followed correctly, built-in types (such as int, string, ...) are allowed. Tuples are allowed too.

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Aug 8, 2018

I don't see a reason to restrict operators

From LDM notes:

Don't allow default as an operand to a unary or binary operator. We need to protect the ability to add new operator overloads in the future.

With the exception of equality. So I'm thinking that the idea is to follow default here.

new() == (1, 2) looks like LDM allowed it after all

To me it's weird because new would be redundant for both tuples and delegates (also from LDM)

(int a, int b) t = new(1, 2); // "new" is redundant
Action a = new(() => {}); // "new" is redundant

(int a, int b) t = new(); // ruled out by "use of struct default ctor"
Action a = new(); // no constructor found

var x = new() == (1, 2); // ruled out by "use of struct default ctor"
var x = new(1, 2) == (1, 2) // "new" is redundant

Can you confirm if this is the intended result?

throw new() (disallow since no type and not null literal)

That might be actually useful (with Exception as the type)? (currently disallowed)

@jcouv

This comment has been minimized.

Copy link
Member Author

jcouv commented Aug 8, 2018

Thanks for the correction. Then let's restrict on operators (like we do for default) :-)

For tuples, the LDM notes are pretty explicit (ie. allow). We could re-discuss.
My view is new(...) isn't useful or desirable for types that have literals (int, string, tuples). But at the same time, it may be strange to block those.

For throw, let's keep it as an open issue as well.

Could you make a PR to update the proposal, including those three points (operators, tuples/literals, throw) as open issues? Thanks. Next LDM will be late August.

@gafter gafter referenced this issue Sep 18, 2018

Open

Champion "Target-typed `new` expression" #100

3 of 5 tasks complete
@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Oct 18, 2018

Not sure if this is already included, but we'll need to adjust type inferrer here.

var info = SemanticModel.GetSymbolInfo(creation.Type, CancellationToken);

Probably there's more, because Type is now TypeSyntax?.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Oct 18, 2018

Oh. We changed the syntax to make somehting that could previous not be null into someting that can be null? That's definitely concerning. No telling how many things that may break in/out of the roslyn codebase. Any analyzers themselves may be screwed on that. Is compat council ok with that approach? I thought it was a no-no in the past...

Any reason this isn't done by introducing something like BaseObjectCreationSyntax, then leaving the existing guy alone (except moving onto that), and then having the new guy derive from that, but have no type-syntax?

That way code has to opt into handling this properly.

Thoughts?

@alrz

This comment has been minimized.

Copy link
Contributor

alrz commented Oct 20, 2018

introducing something like BaseObjectCreationSyntax, then leaving the existing guy alone (except moving onto that), and then having the new guy derive from that, but have no type-syntax?

Not sure if having a "Base" would help with any scenario (if not breaking). I think we can just follow ImplicitArrayCreation (having ImplicitObjectCreation deriving from Expression).

Either way, I agree it's a better approach than just making the field nullable, at least not until we have NRT.

@CyrusNajmabadi

This comment has been minimized.

Copy link
Contributor

CyrusNajmabadi commented Oct 20, 2018

But I agree it's a better approach than just making the field nullable, at least not until we have NRT.

NRT doesn't even help here. Because that presumes that existing code woudl actually be recompiled to even notice this field could now be null. There may be extensions out there that would not and would simply crash on this.

Not sure if having a "Base" would help with any scenario (if not breaking)

Introducing a 'Base' form is actually fine. This is what we did with 'foreach' statements when we added support for the deconstruction form: foreach (var (x, y) in ...

We introduced a CommonForEachStatementSyntax base node and made the existing ForEachStatementSyntax derive from it. We then introduced a new ForEachVariableStatementSyntax for the new form.

I think we can just follow ImplicitArrayCreation (having ImplicitObjectCreation deriving from Expression).

This would also be fine with me.

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