Repository navigation
Unsafe evolution: warn for deprecated feature flag - #86224
Conversation
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
🟡 Changes recommended
Both cases of the existing command-line language-version test still omit CS9402 from their expected diagnostics.
1 open issue
What changed in this PR
This follow-up to #85980 guides users from the legacy unsafe-evolution feature flag to the dedicated MemorySafetyRulesVersion setting.
Changes:
- Adds warning CS9402 when
updated-memory-safety-rulesis present, with replacement MSBuild and compiler options. - Registers the warning at level 2 and adds its message and localization entries.
- Updates compiler and SDK test expectations and adds a multiple-syntax-tree regression test.
The warning uses the existing compilation diagnostic pipeline and keeps the change focused on migration guidance.
| File | Description |
|---|---|
| src/Compilers/CSharp/Test/Syntax/Diagnostics/DiagnosticTest.cs | Verifies warning level 2. |
| src/Compilers/CSharp/Test/CSharp15/UnsafeEvolutionTests.cs | Adds and updates deprecation-warning expectations. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf | Adds Traditional Chinese localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf | Adds Simplified Chinese localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf | Adds Turkish localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf | Adds Russian localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf | Adds Brazilian Portuguese localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf | Adds Polish localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf | Adds Korean localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf | Adds Japanese localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf | Adds Italian localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf | Adds French localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf | Adds Spanish localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf | Adds German localization entries. |
| src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf | Adds Czech localization entries. |
| src/Compilers/CSharp/Portable/Generated/ErrorFacts.Generated.cs | Classifies CS9402 as a warning. |
| src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs | Sets warning level and build-only classification. |
| src/Compilers/CSharp/Portable/Errors/ErrorCode.cs | Assigns diagnostic number 9402. |
| src/Compilers/CSharp/Portable/CSharpResources.resx | Defines the warning message and title. |
| src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs | Reports use of the deprecated flag. |
| src/Compilers/Core/MSBuildTaskTests/SdkIntegrationTests.cs | Expects the warning in SDK integration coverage. |
Review effort: Balanced
There was a problem hiding this comment.
🟢 Approval recommended
The focused warning addition preserves flag semantics, includes consistent diagnostic and resource registration, and has targeted test coverage with no unresolved findings.
No open issues 🎉
1 resolved since last review
Review effort: Balanced
| new CSharpRequiredLanguageVersion(MessageID.IDS_FeatureUnsafeEvolution.RequiredVersion())), Location.None)); | ||
| } | ||
|
|
||
| if (HasFeature(CodeAnalysis.Feature.UpdatedMemorySafetyRules)) |
There was a problem hiding this comment.
This is where we already validate nullable options:
roslyn/src/Compilers/CSharp/Portable/Compilation/CSharpCompilation.cs
Lines 3098 to 3104 in bc72f0c
And where we add errors from CSharpCompilationOptions.ValidateOptions:
So reporting compilation option errors as part of declaration diagnostics is by-design/pre-existing.
There was a problem hiding this comment.
So reporting compilation option errors as part of declaration diagnostics is by-design/pre-existing.
Pre-existing for nullable, perhaps. By-design, doubtful. Options have public API Errors and that is where options' errors should be reported whenever possible. Therefore, if diagnostics related to unsafe can be reported by that API, it should be reported by it. We also should take a look at the nullable related diagnostics.
There was a problem hiding this comment.
Therefore, if diagnostics related to
unsafecan be reported by that API, it should be reported by it.
Sorry, I should be clearer - I think that's not possible since both nullable and unsafe option diagnostics need access to syntax trees which aren't available from ValidateOptions.
|
Done with review pass (commit 2) |
| <value>There is no argument given that corresponds to the required parameter '{0}' of '{1}'</value> | ||
| </data> | ||
| <data name="WRN_DeprecatedFeatureFlag" xml:space="preserve"> | ||
| <value>Feature flag '{0}' is deprecated and will be removed in a future version. Use MSBuild property '{1}' or compiler option '{2}' instead.</value> |
There was a problem hiding this comment.
It seems helpful to mention the project file option that users will likely be using instead of just the compiler option that is rarely used directly. The msbuild project option definition lives in roslyn repo, so it doesn't feel that off to include it here.
| { | ||
| compilation.VerifyDiagnostics(); | ||
| compilation.VerifyDiagnostics( | ||
| // warning CS9402: Feature flag 'updated-memory-safety-rules' is deprecated and will be removed in a future version. Use MSBuild property 'MemorySafetyRulesVersion=2' or compiler option '/memorysafetyrulesversion:2' instead. |
There was a problem hiding this comment.
No, it is not, it is when the Csc task is used.
… rules - An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - `EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
… rules - An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - `EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
… rules - An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - `EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
… rules - An assembly opts in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`, the compiler's own property (dotnet/roslyn#85980). `eng/memorySafetyRules.targets` maps it to the `updated-memory-safety-rules` feature flag until that compiler flows in; delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - `EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010` for `dotnet format`, and `UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` let a migration driver load one target framework of the migrated project. A driver script is at https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
…ollment (#136621) Contributes to #131451. Infrastructure only: no library is migrated here. ## Changes **Migration tooling (`src/tools/illink`, interop generators)** - **`UNSAFE_MIGRATION` instead of `#if DEBUG`.** The analyzers and most fixers were compiled out of Checked, so a Checked toolset ran only the unsafe-context fixer. They're now built in every configuration but Release. - **`UnsafeMigrationFixAllProvider`.** The batch fixer takes the equivalence key from one arbitrary diagnostic. Under `dotnet format`, a single unfixable `IL5005` (e.g. on a pointer signature) turned the whole pass into a silent no-op, and results depended on diagnostic order. Each diagnostic now gets its own fix. `SynchronizeUnsafeContract`'s remove and replace-with-`safe` actions share one key. - **`IL5005` keeps `unsafe` in two more cases.** - On `extern`/`[LibraryImport]` methods: removing it oscillated with the `CS9389`/`SYSLIB1064` fixers. - On members whose inactive `#if` branches contain pointer operations: those branches can rely on the modifier's lexical context in other configurations or in other assemblies compiling the same file (e.g. `Marvin.cs` in CoreLib). - **Unsafe-context fixer.** It also runs under the legacy rules, to repair other consumers of a shared file, and it fixes `CS9402` (address of an unassigned variable, dotnet/roslyn#86017). - **`CS9392` also covers `StructLayout` `Pack`/`Size`** (dotnet/roslyn#85921). Otherwise `IL5005` and the field fixer undo each other. - **v2 detection through `IModuleSymbol.MemorySafetyRulesVersion`** in the ILLink analyzers and the interop and System.Text.Json generators. Reading only the feature flag breaks once `MemorySafetyRulesVersion` is used. A shared `Common/src/Roslyn/MemorySafetyRulesPolyfills.cs` extension property stands in for the API (reflection, with the feature flag as fallback) and replaces three reflection-based copies. Once our Roslyn reference has the API, call sites bind to it and the shim can be deleted. - **Restored `IL5009`/`IL5010` and the add-safety-comment fixer from #131484.** dotnet/roslyn#85842 only changes IDE0380, which covers member modifiers (not blocks, expressions or `safe`), and it isn't in our SDK. The stub is the unsafe-context fixer's `// SAFETY: To be audited` placeholder, and inline `/* SAFETY: */` markers are recognized. **Enrollment (`eng/memorySafetyRules.targets`)** - **Opt in with `<MemorySafetyRulesVersion>2</MemorySafetyRulesVersion>`,** the compiler's own property (dotnet/roslyn#85980). Until that compiler flows in, it maps to the `updated-memory-safety-rules` feature flag. `AllowUnsafeBlocks` stays each library's own decision, since a library without unsafe operations needs neither it nor any `safe`/`unsafe` annotations. Delete the mapping then, since the flag will warn (dotnet/roslyn#86224). The existing ref and System.Text.Json test opt-ins move to the property. - **`EnableUnsafeMigrationAnalyzers=true` enables `IL5005`-`IL5010`** through `eng/unsafeMigration.globalconfig`; they are disabled by default and `dotnet format --diagnostics` doesn't enable them. - **`UnsafeMigrationProject`/`UnsafeMigrationTargetFramework` narrow the migrated project to one target framework.** `dotnet format -f` can't be used: it retargets every referenced project too, which fails for platform-specific TFMs. ## Enrolling a library The driver lives in a gist: https://gist.github.com/EgorBo/456f9e7d3680a5f9c8e54a168a907187. Each step runs `dotnet format analyzers`, and the whole sequence repeats until nothing changes: 1. `CS9377`/`CS0106`: remove invalid `unsafe`. 2. `IL5005`: remove undocumented `unsafe`. 3. `IL5006`/`SYSLIB1064`/`CS9389`/`CS9392`: add the contracts that have to be explicit. 4. `CS9364`-`CS9366`/`CS0764`/`CS9390`: synchronize overrides and partial parts. 5. `CS9360`-`CS9363`/`CS9376`/`CS9402`: introduce unsafe contexts. 6. `IL5009`: add safety comments. ```powershell # 1. Opt in: add <MemorySafetyRulesVersion>2</MemorySafetyRulesVersion> to src\libraries\Foo\src\Foo.csproj, # then build it once so its references exist. .\build.cmd libs -c Release # 2. Migrate, one pass per target framework. .\migrate-unsafe-v2.ps1 src\libraries\Foo\src\Foo.csproj -TargetFrameworks net11.0-windows,net11.0-unix,net11.0,netstandard2.0 # 3. Build every target framework, plus the other consumers of the shared files the script lists. .\dotnet.cmd build src\libraries\Foo\src\Foo.csproj -c Release ``` 4. Audit the result: - Replace each `// SAFETY: To be audited` with the actual reasoning. - Change `unsafe` to `safe` on `[LibraryImport]`/`extern` members whose native side is safe, with a `<safety>` doc. - Write a `<safety>` doc for every `unsafe` member that stays. --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: cf859571-aaeb-4ade-b35e-bf2acc951c30
|
@AlekseyTs @333fred I need another sign off here after merging main and fixing up error code number, thanks |

Follow up on #85980 (comment).
Test plan: #81207
Microsoft Reviewers: Open in CodeFlow