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]: PrintMembersIgnoreAttribute for records #3925

Closed
1 of 4 tasks
Youssef1313 opened this issue Sep 21, 2020 · 7 comments
Closed
1 of 4 tasks

[Proposal]: PrintMembersIgnoreAttribute for records #3925

Youssef1313 opened this issue Sep 21, 2020 · 7 comments

Comments

@Youssef1313
Copy link
Member

Youssef1313 commented Sep 21, 2020

PrintMembersIgnoreAttribute for records

  • Proposed
  • Prototype: Not Started
  • Implementation: Not Started
  • Specification: Not Started

Summary

Currently, there is no way for the developer to exclude specific fields or properties from record's PrintMembers.

Motivation

  1. If a record contains an instance member of its same type, PrintMembers will get called recursively forever. Converting the instance member to be a static is one way to fix this, but being able to completely exclude an instance member seems to be a great addition.

  2. If one of the properties doesn't have an overridden ToString() implementation. It's useless to be printed, the developer might want to exclude it, but in that case marking it as static wouldn't be always an available option.

Detailed design

A new attribute is introduced:

[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public class PrintMembersIgnoreAttribute : Attribute { }

If GenerateMethodBody in SynthesizedRecordPrintMembers found the field/property being decorated with this attribute, it shouldn't include it.

Drawbacks

Alternatives

Unresolved questions

  1. If this attribute is applied to non-record members, should the compiler warn and ignore it, or error it completely?
  2. Should the developer be able to force including statics and constants with a similar attribute (e.g. ForceIncludeInPrintMembersAttribute)?

Design meetings

@syndek
Copy link

syndek commented Sep 21, 2020

If we were going to allow a user to force the inclusion of a member, we could just use one attribute. For example:

[AttributeUsage(AttributeTargets.Field | AttributeTargets.Property)]
public class PrintMembersInclusion : Attribute
{
    public PrintMembersInclusion(bool shouldInclude)
    {
        this.ShouldInclude = shouldInclude;
    }

    public bool ShouldInclude { get; }
}

This could then be applied as follows, with the described behaviour:

record TestRecord
{
    [PrintMembersInclusion(shouldInclude: true)] // Included. Overrides default behaviour for constants.
    public const string PublicConstantString = "This is a string.";

    [PrintMembersInclusion(shouldInclude: true)] // Included. Overrides default behaviour for static readonly fields.
    public static readonly int PublicStaticReadonlyInt = 42;

    [PrintMembersInclusion(shouldInclude: false)] // No effect. Field is already ignored.
    private static readonly int PrivateStaticReadonlyInt = 1788;

    [PrintMembersInclusion(shouldInclude: true)] // No effect. Property is already included.
    public string PublicInstanceString { get; }

    [PrintMembersInclusion(shouldInclude: false)] // Not included. Overrides default behaviour for public properties.
    public int PublicInstanceInt { get; }

    [PrintMembersInclusion(shouldInclude: true)] // Included. Overrides default behaviour for private properties.
    private bool PrivateBool { get; }
}

@HaloFour
Copy link
Contributor

Seems like something that could be handled by a source generator which emits the appropriate PrintMembers method into your record. Having the compiler manage that out-of-the-box would be wading into policy territory that the team has expressed wanting to avoid. I think it's be difficult to argue that the equality should not be influenced based on attributes if something like printing members is.

@333fred
Copy link
Member

333fred commented Sep 21, 2020

I would agree with @HaloFour's sentiment here. This, particularly the potential for stack overflows, was explicitly discussed, and we feel that the scenarios you're going to want to have a custom Equals implementation are either the same or very similar to the scenarios that you'll want a custom ToString implementation (and GetHashCode). imo, this is something best addressed in a source generator.

@Youssef1313
Copy link
Member Author

Thanks @HaloFour and @333fred. This is indeed achievable with source generators.

@Youssef1313
Copy link
Member Author

If anyone is interested, I started working on a simple source generator for this purpose.

333fred added a commit that referenced this issue Sep 27, 2020
* We should Inform people about appropriate use of analyzers

as per #3925

* Update README.md

Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
@stephan-tolksdorf
Copy link

@HaloFour

Seems like something that could be handled by a source generator which emits the appropriate PrintMembers method into your record. Having the compiler manage that out-of-the-box would be wading into policy territory that the team has expressed wanting to avoid. I think it's be difficult to argue that the equality should not be influenced based on attributes if something like printing members is.

The generated Equals and GetHashCode only read fields, while PrintMembers and ToString read all 'printable members (non-static public field and readable property members)', which in particular includes calculated properties. It's easy to shoot yourself with this. I just did with properties for casting purposes, along the following lines:

record TypeA(int Value) {
  public TypeB TypeB => new TypeB(Value);
}

record TypeB(long Value) {
  public TypeA TypeA => new TypeA((int)Value);
}

An attribute for excluding properties would be a convenient way to fix ToString here. I don't understand the slippery slope argument with regard to equality.

@HaloFour
Copy link
Contributor

HaloFour commented Apr 3, 2023

@stephan-tolksdorf

I don't understand the slippery slope argument with regard to equality.

It's not in regard to equality, it is in regard with having policy drive the language's behavior. The compiler offers an escape hatch in that if you provide an implementation of PrintMembers then it will not synthesize one for you, and having a source generator apply the policy you want in order to emit that member automatically is an intentionally supported scenario.

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

No branches or pull requests

5 participants