Add new feature to fix invalid constraints.#59889
Add new feature to fix invalid constraints.#59889CyrusNajmabadi merged 4 commits intodotnet:release/dev17.3from
Conversation
|
I'm surprised this is targeting a specific release branch. Why not target |
.github/CODEOWNERS
Outdated
| docs/ide @dotnet/roslyn-ide | ||
|
|
||
| # Notify docs team for new breaking change documentation | ||
| docs/compilers/CSharp @dotnet/docs |
There was a problem hiding this comment.
This should not be in this PR. It will be removed from main anyways. Likely related to PR targeting release/dev17.3 instead of main.
| enumOrDelegateKeyword = default; | ||
| constraint = diagnostic.Location.FindNode(cancellationToken) as TypeConstraintSyntax; | ||
| if (constraint == null) | ||
| return false; |
There was a problem hiding this comment.
Just to confirm, most of the cases returning false seem unreachable. This is just being defensive, right?
There was a problem hiding this comment.
Correct. Compiler diagnostics come with no contracts/guarantees. They can and do change over time. So consumption code needs to be written to be resilient to that.
| Debug.Assert(result || (_getTypeCache.TryGetValue(fullyQualifiedMetadataName, out var addedType) && ReferenceEquals(addedType, val))); | ||
| Debug.Assert(result | ||
| || !_getTypeCache.TryGetValue(fullyQualifiedMetadataName, out var addedType) // Could fail if the type was already evicted from the cache | ||
| || ReferenceEquals(addedType, val)); |
There was a problem hiding this comment.
Ditto, this change doesn't belong here
| { | ||
| }", | ||
|
|
||
| }.RunAsync(); |
There was a problem hiding this comment.
nit: Blank line above RunAsync. Also in test above
| await new VerifyCS.Test | ||
| { | ||
| TestCode = @" | ||
| class C<T> where T : {|CS9010:enum|} |
There was a problem hiding this comment.
I see we only have tests for single constraint. I think that makes sense because no other constraint would be legal. However, consider testing with some other constraints for completeness.
There was a problem hiding this comment.
sure. happy to add.
| } | ||
|
|
||
| [WpfFact] | ||
| public void TestReturnInSixQuotesMoreQuotesLaterOn() |
There was a problem hiding this comment.
Not sure whether this is meant to be in this PR, or an artifact of targeting 17.3 branch.
| var startDelimeterLength = 0; | ||
| var endDelimeterLength = 0; | ||
| for (int i = token.SpanStart, n = token.Span.End; i < n; position++) | ||
| for (int i = token.SpanStart, n = token.Span.End; i < n; i++) |
jcouv
left a comment
There was a problem hiding this comment.
Done with review pass (iteration 10)
@jcouv I agree. But new features are not allowed in main currently. They have to go into 17.3 |
|
The other stuff got pulled in with retargetting. Once main is merged in, they'll go away |
jcouv
left a comment
There was a problem hiding this comment.
LGTM, but I'll wait until the extra changes no longer appear in diff to sign-off. Thanks
Followup to recent compiler work to detect this mistake and report a custom diagnostic for it.
Looks like this: