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

Proposal: Flow attributes on primary constructor parameters to generated fields #73899

Closed
sbomer opened this issue Jun 7, 2024 · 8 comments
Closed
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead

Comments

@sbomer
Copy link
Member

sbomer commented Jun 7, 2024

In dotnet/runtime#101861 we encountered a scenario where ILLink produces false positive warnings because annotations on a primary constructor parameter aren't present on a generated field. It sounds like we're open to findind a balance between adjusting implementation details to help out ILLink, and not overly constraining the emit strategy. #46646 discusses a similar issue for annotations on generic parameters.

Here's the most straightforward proposal I could think of: we simply copy the annotation from a primary constructor parameter to the generated field/property if possible. Specifically:

  • For record types, a primary ctor parameter annotation gets copied to the generated property only if the attribute type supports AttributeTargets.Property. edit: removed this because record primary ctors support [parameter: ...] attribute specifications.
  • For non-record types, if there is a generated field for a given primary ctor parameter, a parameter annotation gets copied to the generated field only if the attribute type supports AttributeTargets.Field. If there's no generated field, nothing changes.

My hope is that this simple proposal solves the problem without needing any other indication that an annotation should or should not be copied to the field/property. But I don't know if that will cause problems, so I wanted to start a discussion.

The main open questions for me are:

  • Do we know of any attributes with AttributeTargets.Parameter | AttributeTargets.Field (or AttributeTargets.Parameter | AttributeTargets.Property) where the semantics are meaningfully different for parameters vs fields (or parameters vs properties)?
  • Is it a breaking change for a new compiler version to add annotations to a public property?
  • Are there any concerns around guaranteeing the emit strategy for annotations on properties generated for record primary ctor parameters?

@agocke @MichalStrehovsky @jaredpar

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jun 7, 2024
@agocke agocke changed the title Flow attributes on primary constructor parameters to generated fields/properties Proposal: Flow attributes on primary constructor parameters to generated fields/properties Jun 7, 2024
@agocke
Copy link
Member

agocke commented Jun 7, 2024

Here's the most straightforward proposal I could think of: we simply copy the annotation from a primary constructor parameter to the generated field/property if possible.

More precisely, I think the proposal is to copy the attributes if compatible with the AttributeTargets. My understanding is that AttributeTargets aren't actually enforced by the runtime, so it is up to the compiler whether or not to respect them.

we simply copy the annotation from a primary constructor parameter to the generated field/property if possible.

I think the proposal is broader than just primary constructors, right? From broader context, I think the goal would be, for any "compiler-hoisted variable", if the source is a legal attribute target, and the destination is a legal attribute target, and the AttributeTargets on the attribute allow the attribute to appear on the target, the compiler would place an equivalent attribute on the corresponding compiler-generated variable.

@sbomer
Copy link
Member Author

sbomer commented Jun 7, 2024

I think the proposal is broader than just primary constructors, right?

Yes, I agree it makes sense to extend the proposal to all cases like you described. I don't know all the cases, but I can at least think of the following:

  • parameters of async/iterator methods
  • method parameters captured by a lambda or local function

@jaredpar
Copy link
Member

jaredpar commented Jun 8, 2024

I agree we need a broader proposal here vs a specific one for primary constructors. Otherwise we will be back here for closures, records, etc ... Think the better path forward is a broader proposal that lets the IL linker get the info it needs without constraining the compiler from evolving its emit approach

@alrz
Copy link
Contributor

alrz commented Jun 8, 2024

For comparison, F# encodes the mapping itself in attributes (sharplab.io) although it annotates the properties instead of fields.

@agocke
Copy link
Member

agocke commented Jun 10, 2024

@jaredpar Should we make this issue a draft spec, or would you prefer something in dotnet/designs?

@sbomer
Copy link
Member Author

sbomer commented Jun 10, 2024

I just realized C# already supports [property: ...] syntax for annotating record primary constructor parameters, so I don't think we need to consider record types here. That's helpful because it means we don't need to define behavior that affects the public API for primary constructor parameters (which I think would have to be a language spec?).

Are the implicit type parameters of nested types considered an implementation detail, or behavior specified by the language? For example, this prints System.Int32:

G<int>.N.M();
class G<T> {
    public class N {
        public static void M() {
            foreach (var p in typeof(N).GetGenericArguments())
                Console.WriteLine(p);
        }
    }
}

@sbomer sbomer changed the title Proposal: Flow attributes on primary constructor parameters to generated fields/properties Proposal: Flow attributes on primary constructor parameters to generated fields Jun 10, 2024
@sbomer
Copy link
Member Author

sbomer commented Jun 10, 2024

I opened a broader version of the proposal at #73920. Happy to move it elsewhere if desired.

@sbomer
Copy link
Member Author

sbomer commented Jun 10, 2024

Closing this - we can continue discussion in the broader proposal that's a superset of this one: #73920.

@sbomer sbomer closed this as completed Jun 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead
Projects
None yet
Development

No branches or pull requests

4 participants