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

Test plan for ref fields #59194

Closed
jcouv opened this issue Feb 1, 2022 · 2 comments
Closed

Test plan for ref fields #59194

jcouv opened this issue Feb 1, 2022 · 2 comments

Comments

@jcouv
Copy link
Member

jcouv commented Feb 1, 2022

Spec: https://github.com/dotnet/csharplang/blob/main/proposals/low-level-struct-improvements.md
Tracking issue in runtime repo: dotnet/runtime#63768
Championed issue: dotnet/csharplang#1147

Guideline for addressing remaining issues:

  • Base local testing off of 17.4 branch (verify behavior shipping in .NET 7)
  • Base PRs on main (verify behavior in VS 17.5).
  • Issues which we plan to address in 17.5, open new standalone issues and check-off in here
  • Scenarios which we do not plan to test, either delete the line or add ❌ at start of line at your discretion.

Compiler

  • spec: add grammar
  • spec: scoped can only be applied to a ref type or a type that is a ref struct
  • import of ref field when runtime feature flag is missing: RefFieldTests.RuntimeFeature
  • ref assemblies and consumption by old compilers
    • ref fields are emitted as ref for reference assemblies (no change). Note that our ability to control what old compilers actually do with these fields is somewhat limited. We prefer if it just gives a use site error when the ref field is used, but may not be able to make changes if the behavior does not end up being ideal. (RefFieldTests.RefAssembly)
    • ref assemblies which contain ref fields not consumable by old compilers (even if ref field is private) (not a supported scenario)
    • Test metadata compatibility with previous compiler release (C# and VB) (not a supported scenario)
  • check internal enumerations of local variable kinds (ensure that all LocalDeclarationKind values which can by by-ref or ref struct are given correct val escape and ref escape, with or without scoped keyword) Support scoped modifier for other local declarations #62039
  • spec: OHI
  • manual test of C++/CLI (can reference local assemblies with ref fields) (known issue, tracked by C++/CLI team)
  • spec
  • add to compiler test plan
  • -langversion:11
    • for ref field declarations in source (also requires RuntimeFeature.ByRefFields)
    • for use-site of ref fields from metadata or compilation reference (RefFieldTests.LanguageVersionDiagnostics)
  • updated escape rules enabled by
  • ref and ref readonly fields for ref struct only
  • field must be declared readonly ref in readonly ref struct
  • escape analysis within readonly methods (RefFieldTests.MethodArgumentsMustMatch_13)
  • readonly ref emitted as initonly
  • ref readonly emitted with System.Runtime.CompilerServices.IsReadOnlyAttribute
  • ref struct with a ref field is not considered unmanaged stackalloc should be disallowed for structs with ref fields #63104
  • field initializers:
  • ref field modifiers
    • static ref fields disallowed
    • const ref fields disallowed
    • volatile ref fields disallowed
    • fixed ref fields disallowed
  • ref auto-properties disallowed
    ref T P0 { get; }       // error
    ref T P1 { get; set; }  // error
    ref T P2 { get; init; } // error
  • ref in object initializers supported: new S { F = ref t } Support ref field assignment in object initializers #62120
  • ref field of the containing type: Allow ref field of the containing type #62098
    ref struct R<T>
    {
        public ref R<T> Next;
    }
  • _ = s.F; should throw NullReferenceException if ref field is null - see Record read of ref local even if value is discarded #60910 (comment)
  • scoped as type name
    • supported with -langversion:10 and earlier for compat
    • error with -langversion:11
  • scoped modifier roundtripped to metadata
  • scoped considered in overrides and interface implementation
  • conversion errors for mismatch of scoped
  • scoped included in inferred delegate types: var f = (R x, scoped R y) => x;
  • scoped parameters
    • ref, in, out
    • scoped this in extension method
    • params scoped
  • scoped locals
    • ref, ref readonly
    • const
    • foreach iteration variable
    • using variable
    • deconstruction declaration
    • out declaration
    • var and ref var
  • local with/without scoped modifier with scoped/unscoped initializer
  • ScopedRefAttribute changes scoped default when targeting:
    • ref to ref struct parameters
    • out parameters
  • ScopedRefAttribute disallowed in source Report error if ScopedRefAttribute is used directly in source #62124
  • UnscopedRefAttribute:
  • updated escape rules
    • ref-safe-to-escape for ref fields
    • ref reassignment
    • ref-safe-to-escape and safe-to-escape for scoped, scoped ref, ref scoped parameters and locals
    • struct this is implicitly scoped ref T
    • out parameters are implicitly scoped out
    • scoped parameters affect argument contribution for ref-safe-to-escape and safe-to-escape of method invocation
    • scoped parameters affect "method arguments must match" rule (arg "mixing")
  • warn when a parameter could be marked scoped Report warning for unannotated parameters that could be marked scoped #64344
  • SymbolDisplay
    • ref qualifier and ref custom modifiers
    • scoped and scoped ref for parameters and locals
    • out parameters (should we show the implicit scoped?)
  • document breaking changes:
    • cannot return out parameter by reference
    • rvalue from method invocation that returns a ref struct is safe-to-escape from ... the ref-safe-to-escape of all ref arguments
  • Test when scoped ref or scoped in cannot be passed as an argument to an ordinary ref/in parameter.
    • when the target method also returns ref struct (RefFieldTests.ReturnRefFieldByValue)
    • when the target method also returns ref/ref readonly (RefFieldTests.ReturnRefFieldByRef_03)
    • when the target method also has a ref parameter whose type is ref struct (RefFieldTests.MethodArgumentsMustMatch_03)
  • auto-default: constructor must implicitly ref-assign a null-ref to ref fields which are not explicitly ref-assigned before all usages and returns. Auto-default ref fields with a null ref-assignment #63065
  • cannot make an anonymous type with ref members
  • disallow void F([UnscopedRef] scoped ref int i) { } (RefFieldTests.UnscopedScoped)
  • allow updated escape rules with either -langversion:11 or RuntimeFeature.ByRefFields (i.e. NET 7) (RefFieldTests.UseUpdatedEscapeRules)
  • required ref fields (RefFieldTests.RequiredField_01, _02)
  • unskip test RefFieldTests.InitRefField_UnsafeNullRef
  • unskip tests from RefFieldTests.vb

Public API

Productivity

Open questions

  • diagnostic for scoped modifier when ref or ref struct value cannot escape?
    static void F1<T>(scoped ref T t) { ... }   // warning: scoped is unnecessary
    static void F2<T>(scoped Span<T> s) { ... } // warning: scoped is unnecessary
    Answer: don't give such a diagnostic, it will be inconvenient and disruptive to those who use scoped ref on refs which do not escape as a matter of style.
  • diagnostic for scoped out since out parameters are implicitly scoped? Answer: No, unless we hear further interest in such behavior.
  • should we emit the [ScopedRef] attribute in metadata for an out parameter? The spec says it's implicitly scoped out. Answer: Don't emit ScopedRef when parameters are implicitly scoped.
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 1, 2022
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Feb 17, 2022
@jcouv jcouv added this to the Compiler.Next milestone Feb 17, 2022
@jcouv jcouv modified the milestones: Compiler.Next, C# 11.0 Apr 28, 2022
@jaredpar jaredpar modified the milestones: C# 11.0, 17.4 Jun 24, 2022
@RikkiGibson
Copy link
Contributor

RikkiGibson commented Oct 21, 2022

Test plan contains the following item.

  • QuickInfo for out parameters (should we show the implicit scoped?)

i.e. if I have a method void M(out int x) { } should it display in Quick Info as void M(scoped out int x). My gut feeling here is no. @sharwell @dotnet/roslyn-ide for input.

@RikkiGibson
Copy link
Contributor

We've addressed everything needed for .NET 7 and moved everything else out to separate issues. Closing out the test plan.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants