Unsafe evolution: add ISymbol public API - #84674
Conversation
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR extends the Microsoft.CodeAnalysis.ISymbol public API surface by adding a new experimental RequiresUnsafe property and wires it through relevant symbol wrappers/implementations across C#, VB, workspace code-generation symbols, and Metadata-as-Source, with compiler tests and reference-API updates to keep tooling in sync.
Changes:
- Added experimental
ISymbol.RequiresUnsafeAPI and updated public API baselines. - Implemented/plumbed
RequiresUnsafethrough C# public model symbols, VB symbols (defaultFalse), Metadata-as-Source wrapper symbols, and code generation symbols. - Added C# compiler tests validating
RequiresUnsafevalues and ensured semantic search reference API lists include the new getter.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/Symbols/CodeGenerationSymbol.cs | Adds RequiresUnsafe stub implementation for code-generation symbols. |
| src/Tools/SemanticSearch/ReferenceAssemblies/Apis/Microsoft.CodeAnalysis.txt | Updates reference API list to include ISymbol.get_RequiresUnsafe. |
| src/Features/Core/Portable/MetadataAsSource/AbstractMetadataAsSourceService.AbstractWrappedSymbol.cs | Forwards RequiresUnsafe through Metadata-as-Source wrapped symbols. |
| src/Compilers/VisualBasic/Portable/Symbols/Symbol.vb | Adds default VB implementation of ISymbol.RequiresUnsafe returning False. |
| src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs | Adds/extends tests asserting RequiresUnsafe behavior for public symbols. |
| src/Compilers/CSharp/Portable/Symbols/Symbol.cs | Removes now-obsolete comment about needing a public API for caller-unsafe exposure. |
| src/Compilers/CSharp/Portable/Symbols/PublicModel/Symbol.cs | Implements ISymbol.RequiresUnsafe in the C# public symbol wrapper via caller-unsafe mode. |
| src/Compilers/CSharp/Portable/Symbols/PublicModel/PreprocessingSymbol.cs | Implements RequiresUnsafe as false for preprocessing symbols. |
| src/Compilers/Core/Portable/Symbols/ISymbol.cs | Adds the new experimental RequiresUnsafe property and XML doc. |
| src/Compilers/Core/Portable/PublicAPI.Unshipped.txt | Records the new experimental public API member. |
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
@333fred @AlekseyTs for reviews, thanks |
Consider aligning the name with the current spec terminology ("requires-unsafe"). Not necessarily in this PR. #Closed Refers to: src/Compilers/CSharp/Portable/Symbols/Symbol.cs:627 in 835cab0. [](commit_id = 835cab0, deletion_comment = False) |
|
Done with review pass (commit 1) |
|
This PR modifies public API files. Please follow the instructions at https://github.com/dotnet/roslyn/blob/main/docs/contributing/API%20Review%20Process.md for ensuring all public APIs are reviewed before merging. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/Compilers/Core/Portable/Symbols/ISymbol.cs:165
- The XML doc for
ISymbol.RequiresUnsafehas awkward/incorrect phrasing (“considered requires-unsafe”). Rewording this improves clarity for API consumers without changing meaning.
/// Whether this symbol is considered requires-<see langword="unsafe"/> under the updated memory safety rules,
/// i.e., the symbol requires an <see langword="unsafe"/> context at its use site.
/// This can be either a symbol compiled with the updated memory safety rules which has <see langword="unsafe"/> in its signature,
/// or a symbol compiled with the legacy memory safety rules which has pointers in its signature.
|
|
||
| Private ReadOnly Property ISymbol_RequiresUnsafe As Boolean Implements ISymbol.RequiresUnsafe | ||
| Get | ||
| Return False |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
src/Compilers/Core/Portable/Symbols/ISymbol.cs:166
- The opening sentence is grammatically awkward ("considered requires-unsafe") and also reads as if legacy-pointer cases are still "under the updated rules". Rewording to directly describe the observable behavior makes the API contract clearer.
/// Whether this symbol is considered requires-<see langword="unsafe"/> under the updated memory safety rules,
/// i.e., the symbol requires an <see langword="unsafe"/> context at its use site.
/// This can be either a symbol compiled with the updated memory safety rules which has <see langword="unsafe"/> in its signature,
/// or a symbol compiled with the legacy memory safety rules which has pointers in its signature.
/// </summary>
src/Workspaces/SharedUtilitiesAndExtensions/Workspace/Core/CodeGeneration/Symbols/CodeGenerationSymbol.cs:136
- The CA1822 suppression can be avoided by implementing
RequiresUnsafeas an explicitISymbolmember. That removes the need for a#pragmawhile still satisfying the interface and keeping behavior the same (alwaysfalse).
// CA1822 suppression is needed until all projects using this file are updated to newest Roslyn which has this member on ISymbol.
#pragma warning disable CA1822 // Mark members as static
public bool RequiresUnsafe => false;
#pragma warning restore CA1822
src/Compilers/CSharp/Portable/Symbols/Symbol.cs:623
- This comment says we’re only tracking a terminology change, but the XML docs immediately below still describe "caller-unsafe" and the method is still named
GetCallerUnsafeMode. Suggest updating the comment to reflect the current purpose (public API exposure) so it doesn’t become misleading/stale.
// https://github.com/dotnet/roslyn/issues/82546: change terminology from "caller unsafe" to "requires unsafe"
Part of #82791.
Test plan: #81207
@EgorBo FYI
Microsoft Reviewers: Open in CodeFlow