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 "field keyword in properties" #57012

Open
jcouv opened this issue Oct 7, 2021 · 5 comments
Open

Test plan for "field keyword in properties" #57012

jcouv opened this issue Oct 7, 2021 · 5 comments

Comments

@jcouv
Copy link
Member

jcouv commented Oct 7, 2021

Championed proposal: dotnet/csharplang#140
Spec: proposals/field-keyword.md

public string PropertyConstraint
{
    get;
    set => field = value ?? throw new ArgumentNullException();
} = "";

Compiler:

  • language version
  • field keyword
    • based on language version
    • as primary_expression only
    • escaped with @
    • this.field, base.field, Qualifier.field
    • in simple property: object P => expr;
    • in property accessors: expression body; block body
    • in indexer accessors
    • in event accessors
    • in nested lambdas and local functions
    • in property initializer
    • in attributes on property, accessor, nested function
    • in nested function signature as default value
    • in nameof()
    • in call to [Conditional]
  • warning when previously binding to other symbol
    • based on language version
    • member of containing type, base type
    • local, in same or nested function
    • parameter in nested function
    • type parameter, in containing type or nested function
  • mixed auto- and manually-implemented accessors (see FieldKeywordTests.ImplicitAccessorBody_*)
    • get, set, init
    • in interface
  • field use
    • static and instance properties on class, struct, ref struct, interface, record*
    • get-only, set-only, init-only (see FieldKeywordTests.AutoPropertyMustHaveGetAccessor_*)
    • field references in multiple properties: each get private backing field
    • auto-implemented properties (including field) must override all accessors (see FieldKeywordTests.Override_*, New_*)
    • in nested function, including static field reference in static function (see FieldKeywordTests.Lambda_*, LocalFunction_*)
    • in attribute arguments (see FieldKeywordTests.Attribute_*)
    • ref returning property (see FieldKeywordTests.RefReturning_*)
    • properties with restricted types (see FieldKeywordTests.RestrictedTypes)
    • properties with by-ref-like types (see FieldKeywordTests.ByRefLikeType_*)
    • @field with and without backing field
    • Color Color
  • synthesized backing field
    • backing field emitted when any auto-implemented accessor or field reference
    • backing field marked [CompilerGenerated] (see FieldKeywordTests.Attribute_03)
    • emitted when used in [Conditional] code only (see FieldKeywordTests.Conditional) (see open LDM question)
          object P { set { Debug.Assert(field is null); } }
    • emitted to ref assembly
  • [field:] attributes
    • emitted on synthesized backing field (see FieldKeywordTests.Attribute_03)
    • not emitted on property
    • error if no auto-implemented accessors and no field references (see FieldKeywordTests.PartialProperty_Attribute_03)
    • [field: Obsolete]
  • nameof(field)
    • nameof(field) as invocation
  • property initializer (see FieldKeywordTests.Initializer_*)
    • allowed when any auto-implemented accessor or field reference
    • static and instance properties on class, struct, ref struct, interface
    • written to field directly
  • constructor assignment (see FieldKeywordTests.ConstructorAssignment_*)
    • assigns to setter if available, otherwise getter
    • static and instance properties on class, struct, ref struct, interface
    • assigned in other initializer
    • assigned in nested function
  • flow analysis
    • warning CS0414: The field is assigned but its value is never used
    • warning CS0649: Field is never assigned to, and will always have its default value
    • auto-default initialization of struct backing fields and warnings when property is accessed in constructor (see FieldKeywordTests.DefaultInitialization_*)
      • expect no auto-default inserted when a field initializer is used
      • expect an auto-default when assigning in constructor
  • readonly properties (see FieldKeywordTests.ReadOnly_*)
    • backing field is initonly when type, property, or accessor is readonly
    • error reporting when modifying field in get, set accessors
  • set accessor in readonly context for property using field (see open LDM question)
  • partial properties (see FieldKeywordTests.PartialProperty_*)
    • mix of auto- and manually-implemented accessors
    • static and instance properties on class, struct, ref struct, interface, record*
    • field in accessors
    • initializer on definition or implementation
    • initializer on definition and implementation with same or different values
    • initializer with no field or auto-implementation
    • field references on definition or implementation
    • field references on definition and implementation
    • field references on readonly property or accessor with same or different modifiers on parts
    • [field:] attributes concatenated across parts
      • error when the attributes are the same and AllowMultiple=false
    • partial properties where the implementation uses field and one or both parts have field: targeted attribute lists. (See also PartialPropertiesTests.Attributes_Property_BackingField. This issue referenced in source.)
    • Check warning WRN_SequentialOnPartialClass is produced when needed (see also comment)
    • comment from partial properties
    • comment from partial properties
  • nullability (initial implementation) (see FieldKeywordTests.Nullability_*)
    • field has annotations of containing type
    • [field:MaybeNull] etc. affect field
  • SemanticModel
    • GetSymbolInfo(fieldExpr) returns backing field
    • field in the speculative model does not bind to a backing field if the original location does not have a backing field
    • IOperation for expression referencing field
  • struct fulfills unmanaged constraint and managed type rules (see pointer types) depending on synthesized fields
  • ref safety:
    struct S
    {
        [UnscopedRef]
        Span<int> Prop1 => M(ref field); // should not error
    
        // Should error
        Span<int> Prop2 => M(ref field); // should error
      
        static Span<int> M(ref Span<int> span) => default;
    }
  • public API review: [API proposal] Syntax changes for field keyword #74937
  • update compiler test plan

Productivity:

  • Fixer for compat break
  • QuickInfo
  • Completion
  • Colorization
  • Conversion?
  • F1 help on field
  • EnC
@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 7, 2021
@jcouv jcouv removed the untriaged Issues and PRs which have not yet been triaged by a lead label Oct 7, 2021
@jcouv jcouv added this to the 17.1 milestone Oct 7, 2021
@CyrusNajmabadi CyrusNajmabadi modified the milestones: 17.1, 17.2 Dec 16, 2021
@CyrusNajmabadi
Copy link
Member

Moving to 17.2. The work for thsi hasn't gotten merged in yet anyways.

@Youssef1313
Copy link
Member

Note to self: The following properties for SynthesizedBackingFieldSymbol needs to be revised:

  • DeclaringSyntaxReferences (should it remain empty or return the first usage of field keyword?)
  • IsImplicitlyDeclared (should it always return true or return false for field-keyword backing fields?)

@Youssef1313
Copy link
Member

Youssef1313 commented Apr 28, 2022

There are interesting scenarios around overriding virtual properties.

public class Base
{
    public virtual int P1 { get; set; }
    public virtual int P2 { get; set; }
    public virtual int P3 { get; set; }
    public virtual int P4 { get; set; }
    public virtual int P5 { get; set; }

    public virtual int P6 { get => 0; set { } }
}

public class Derived : Base
{
    // no error.
    public override int P1 { get => 0; }
    
    // This produces Auto-implemented properties must have get accessors.
    // However, per the spec updates, a set-only with semicolon is allowed.
    // Should this now produce an error similar to ERR_AutoPropertyMustOverrideSet?
    public override int P2 { set; }

    // This synthesizes a sealed setter. Make sure we have this behavior for semi-auto.
    public sealed override int P3 { get => 0; }

    // Auto-implemented properties must override all accessors of the overridden property.
    public override int P4 { get; }

    // Will this be allowed? It's not allowed for old-style auto properties (see P4).
    public override int P5 { get => field; }

    public override int P6 { get => field; }

    public Derived()
    {
        // P6 looks like a property that's assignable through backing field.
        // But this shouldn't be the case due to the inherited setter.
        // We should use 'GetOwnOrInheritedSetMethod' for the logic of assignability through backing field.
        // Add a test to confirm that.
        P6 = 10;
    }
}

Do these scenarios have clear answer as to what's the expected behavior? Are there any that needs to be discussed in LDM?

cc @AlekseyTs @333fred @CyrusNajmabadi

@AlekseyTs
Copy link
Contributor

per the spec updates, a set-only with semicolon is allowed.

Could you add a quote from the relevant portion of the spec?

Should this now produce an error similar to ERR_AutoPropertyMustOverrideSet?

Please include exact wording of the message.

// Will this be allowed? It's not allowed for old-style auto properties (see P4).

We probably should treat this as a property with a user provided body. Presence of the field keyword probably shouldn't make any difference.

@AlekseyTs
Copy link
Contributor

@Youssef1313 BTW, consider not "polluting" the Test Plan issue with "random" discussions. Open a separate issue for each specific question.

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

7 participants