Address test plan for target-typed new

[alrz]

Follow-up #25196
Test plan: #28489

[alrz]

Please review #25196 before reviewing this.

[alrz]

Binary operators are not wholly tested yet. Pending design decision on how we want them to behave.

See #28489 (comment)

[gafter]

Could you rebase please? I merged the previous PR, and for some reason those deltas are showing up here.

[jcouv]

                Error(diagnostics, ErrorCode.ERR_NewWithTupleTypeSyntax, syntax, type);

So, I got a bit confused on tuples. Latest LDM notes say they should be allowed.
https://github.com/dotnet/csharplang/blob/master/meetings/2018/LDM-2018-06-25.md#unconstructable-types

Can you fix or add a PROTOTYPE(target-typed-new): ... comment as a reminder? #Closed

---

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Conversions.cs:230 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[jcouv]

            Error(diagnostics, ErrorCode.ERR_BadOpOnNullOrDefault, node, operatorText, operand.Display);

nit: consider renaming ERR_BadOpOnNullOrDefault. Maybe ERR_BadOpOnTypelessExpression. #Closed

---

Refers to: src/Compilers/CSharp/Portable/Binder/Binder_Operators.cs:2279 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[jcouv]

Code change looks good (iteration 4). I'll finish reviewing the tests tomorrow. Thanks

[jcouv]

}

Consider restoring the compilation/execution/validation of the expressions.

---

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/TargetTypedNewTests.cs:58 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[jcouv]

    public void TestArguments_Params()

If not already tested, another params scenario would be interesting: void M(params int[] p) invoked with M(new()) or M(new[] { new() }). (I expect errors in both cases)

Thinking about array initializers, here's another idea: new int[] { new() } (ok)

---

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/TargetTypedNewTests.cs:172 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[jcouv]

    M((new(), new()), new C());

Nice test case! #Closed

---

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/TargetTypedNewTests.cs:288 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[jcouv]

        comp.VerifyEmitDiagnostics();

Consider putting CompileAndVerify back (with some verification of data stored in dictionaries). #Closed

---

Refers to: src/Compilers/CSharp/Test/Semantic/Semantics/TargetTypedNewTests.cs:527 in 30507d5. [](commit_id = 30507d5, deletion_comment = False)

[alrz]

            // The old native compiler ignores ref/out in a delegate creation expression.
            // For compatibility we implement the same bug except in strict mode.
            // Note: Some others should still be rejected when ref/out present. See RefMustBeObeyed.

Do we need to do this in new()? since it's for compatibility, I think it's safe to not do that. Also, doing so would complicate argument binding because we bind arguments early before knowing the type.

[gafter]

Looks great. We'll turn the PROTOTYPE comments into an issue and deal with that separately.

[jcouv]

LGTM Thanks

[jcouv]

@gafter for second sign-off. Thanks

I'll close and re-open the PR to kick CI.

[jcouv]

I'll close and re-open the PR to kick CI.

[jcouv]

We'll need to refresh the features/target-typed-new branch with latest bits from master before we can merge this PR (that's why CI isn't working in this PR).

[gafter]

[alrz]

I attempted a merge from compilerNext and turns out I'll have to read the parser from the top.

I noticed there's an assert to avoid lookaheads for new expressions,

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

Line 5874 in 5620c8f

Debug.Assert(mode != ParseTypeMode.NewExpression);

@gafter do you intend to keep this for new() as well? iow with the new parser logic, do we still need lookaheads for this work?

[gafter]

I don't recall the reason for the restriction/assertion. I believe the parser does require look-ahead for parsing types in patterns.