Remove invalid implicit downcast

[sharwell]

Currently SeparatedSyntaxList<SyntaxNode> can be implicitly cast to SeparatedSyntaxList<T> for any syntax type T. This is not a safe or meaningful conversion, so this change seeks to remove it.

• The implicit operator is left in place using its underlying metadata signature to preserve binary compatibility with previous builds
• The implicit operator is marked obsolete to prevent future use of this unsafe method
• No explicit operator could be added because the implicit operator was not removed, so a CastDown method is added to provide the functionality via an explicit method An explicit operator alongside the op_Implicit method using the operator syntax.

[jcouv]

Given that there was no activity on this PR in a while, I'm marking it as "for personal review" for now (so it doesn't show up in our daily list of PRs to review). Thanks

[sharwell]

@jcouv this has been updated again

[AlekseyTs]

• The implicit operator is left in place to preserve binary compatibility with previous builds
• The implicit operator is marked obsolete to prevent future use of this unsafe method
• No explicit operator could be added because the implicit operator was not removed, so a CastDown method is added to provide the functionality via an explicit method

Have you considered replacing the implicit operator with a regular method with the same name an signature instead? That should take care of binary compatibility. Then we can add an explicit operator.

[sharwell]

Have you considered replacing the implicit operator with a regular method with the same name an signature instead? That should take care of binary compatibility. Then we can add an explicit operator.

💭 No, but I'll try now

[CyrusNajmabadi]

Have you considered replacing the implicit operator with a regular method with the same name an signature instead? That should take care of binary compatibility. Then we can add an explicit operator.

TIL! :-)

[sharwell]

@AlekseyTs Now updated to preserve the implicit operator using its metadata name instead of implicit operator.

[AlekseyTs]

Is this change primarily related to the stated goal of the PR? If not, please consider separating it into its own PR.

[sharwell]

It is related. SyntaxListBuilder produced a SyntaxList<SyntaxNode> which needs an explicit downcast. SyntaxListBuilder<TNode> produces SyntaxList<TNode> and removes the need for a downcast.

[AlekseyTs]

Changes under Compilers LGTM, modulo possibly unrelated change in CSharpSyntaxRewriter.cs

[AlekseyTs]

Changes under Compilers LGTM (commit 9)

[AlekseyTs]

@dotnet/roslyn-compiler For the second review.

[333fred]

Changes under compilers look fine, but not signing off until build and test issues are corrected.

[333fred]

Compiler changes LGTM (commit 11)

[jaredpar]

@sharwell this PR has appropriate sign offs. It's a bit stale so we probably want to re-run CI here. But are you intending to move forward with this? If not I would like to close to get it off our review queue.

[jaredpar]

Have not heard back so moving to draft.

[sharwell]

@jaredpar Sorry got hit by the command line build failures when I tried to update. Trying again locally and hopefully should work out.