-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Fix superfluous managed address errors for pointer to non-existent type #80760
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
Conversation
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
| [Fact, WorkItem("https://github.com/dotnet/roslyn/issues/36877")] | ||
| public void PointerToNonExistentType_NoSuperfluousError() | ||
| { | ||
| var source = @" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot please use raw string literals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated to use raw string literals in commit 7d8a30f.
| var pointedAtType = ((PointerTypeSymbol)type).PointedAtType; | ||
| if (!pointedAtType.IsErrorType()) | ||
| { | ||
| Binder.CheckManagedAddr(args.CurrentCompilation, pointedAtType, args.Location, args.Diagnostics); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot would it make more sense to put the IsErrorType check inside CheckManaged address? If not, why not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion! Moved the IsErrorType check into CheckManagedAddr in commit 7d8a30f. This centralizes the fix and automatically handles all call sites, making the code cleaner and more maintainable.
| // (4,31): error CS0246: The type or namespace name 'Type' could not be found (are you missing a using directive or an assembly reference?) | ||
| // static unsafe void Method(Type *type) | ||
| Diagnostic(ErrorCode.ERR_SingleTypeNameNotFound, "Type").WithArguments("Type").WithLocation(4, 31) | ||
| // Should not also report CS8500 or CS0208 for managed address of non-existent type |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot no need for this comment. And the same with the remainder of the tests. The last paren can also be pulled up to the same line that WithLocation(...) is on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed unnecessary comments and improved formatting in commit 7d8a30f.
…raw string literals, remove comments Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
@copilot the following 4 tests are failing, now that that warning is no longer being reported. can you fix them all up: Microsoft.CodeAnalysis.CSharp.UnitTests.FileModifierParsingTests.TypeNamedFile_03_CSharp11 |
…ssing types Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
Fixed all 4 tests in commit 5c51e77. Refined the IsErrorType check to only skip the warning for truly missing types (LookupResultKind.Empty or NotATypeOrNamespace), while still reporting it for types that exist but have other issues (e.g., inaccessible types). Updated test expectations for PointerTypeInDeconstruction and TypeNamedFile_03_CSharp11 where error types from parse errors no longer get the warning. |
| var source = """ | ||
| class Program | ||
| { | ||
| static unsafe void Method(Type *type) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot unlike in C++ in c# we put the asterix next to the type not next to the name. so Type* type. Can you fixup all these tests to follow that style.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the pointer syntax style to use Type* type instead of Type *type in all 4 test cases in commit b8f9c2a.
Co-authored-by: CyrusNajmabadi <4564579+CyrusNajmabadi@users.noreply.github.com>
|
@dotnet/roslyn-compiler ptal |
| { | ||
| // Skip the check for error types that represent truly missing types (not found), | ||
| // but still report for error types due to other issues (e.g., inaccessibility). | ||
| if (type is ErrorTypeSymbol { ResultKind: LookupResultKind.Empty or LookupResultKind.NotATypeOrNamespace }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are there tests that observe significance of this condition? I.e. they would fail if the condition was removed.
Still would like to get an answer, pointing to specific test(s)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of demonstrative tests in 7242579
|
Done with review pass (commit 11) |
|
Done with review pass (commit 12) |
…uous-pointer-errors
|
Anything more needed here? |
AlekseyTs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (commit 14)
|
@RikkiGibson @jjonescz ptal :) |
Fix superfluous managed address errors for pointer to non-existent type
Summary
Fixed issue #38378 where declaring a pointer to a non-existent type would report both CS0246 (type not found) and CS8500/CS0208 (managed address warning/error). Now only CS0246 is reported.
Changes Made
CheckManagedAddr()to skip the warning only for truly missing types (LookupResultKind.Empty or NotATypeOrNamespace), but still report for other error scenarios like inaccessible typesTest Results
Fix Pattern
Centralized the check in
CheckManagedAddrwith refined logic: skip warning only for types that are truly missing (Empty/NotATypeOrNamespace), but still report for other error scenarios (e.g., Inaccessible types). Uses modern C# pattern matching syntax for clean code.Original prompt
Fixes #38378
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.