Skip to content
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

[Strict] Do not ignore private reference fields of structs for definite assignment #30194

Closed
gafter opened this issue Sep 27, 2018 · 3 comments · Fixed by #46221
Closed

[Strict] Do not ignore private reference fields of structs for definite assignment #30194

gafter opened this issue Sep 27, 2018 · 3 comments · Fixed by #46221

Comments

@gafter
Copy link
Member

gafter commented Sep 27, 2018

In Dev12 the C# compiler ignored private fields of reference type in a value type imported from metadata when computing definite assignment. Roslyn duplicates that bug for compatibility. In "strict" mode we do not duplicate that bug.

Consider enabling this bug to be fixed in the first warning wave.

@gafter
Copy link
Member Author

gafter commented Jul 13, 2020

Here is a first draft of a proposal for this (to address dotnet/csharplang#3431)

There are two ways of doing data flow analysis. The strong version includes tracking inaccessible fields of structs. The weak version ignores inaccessible fields. (The precise rule is a bit more subtle. I believe we only ignore fields of reference type in the weak version). The compiler currently runs the weak version except in “strict” mode, in which case we run the strong version.

We propose to make the following changes in the C# 9 (dotnet 5) timeframe.

  1. If a method has the [SkipLocalsInit] attribute, or the compilation has the "strict" feature enabled, we use the strong version of analysis for its body. Otherwise
  2. If a sufficiently high /warnversion (>= 5) is enabled, we run the strong version of analysis, and
    1. If it reports no diagnostics, we are done.
    2. If it reports diagnostics, we run the weak version of analysis. Errors that are common to the two runs are reported as errors, and errors only reported by the strong version are downgraded to a warning.
  3. Otherwise we run the weak version of analysis.

@gafter gafter moved this from 16.8 Burndown to In Review in Compiler: Gafter Jul 14, 2020
@gafter
Copy link
Member Author

gafter commented Jul 14, 2020

A simpler alternative proposal would be

If any of the following conditions are true, we run the strong version of analysis

  • the method has the [SkipLocalsInit] attribute
  • the compilation has the "strict" feature enabled
  • a sufficiently high /warnversion (>= 5) is enabled

otherwise we run the weak version of analysis.

@gafter gafter moved this from In Review to 16.8 Burndown in Compiler: Gafter Jul 21, 2020
@gafter
Copy link
Member Author

gafter commented Jul 21, 2020

We settled on the first approach. However, we will not treat [SkipLocalsInit], "strict" mode, or warning waves differently. We will always use the two-pass approach when there are errors, and the downgraded warning will have a warning level of 5. In other words:

  • we (always) run the strong version of analysis, and If it reports no diagnostics, we are done. Otherwise
  • We run the weak version of analysis.
    • Errors that are common to the two runs are reported as errors
    • Errors only reported by the strong version are downgraded to a warning.

gafter pushed a commit to gafter/roslyn that referenced this issue Jul 22, 2020
@gafter gafter moved this from 16.8 Burndown to In Review in Compiler: Gafter Jul 22, 2020
@gafter gafter added the 4 - In Review A fix for the issue is submitted for review. label Jul 22, 2020
gafter pushed a commit to gafter/roslyn that referenced this issue Jul 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment