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

Quick info on field exposes generated member name, unlike on accessors #75529

Open
jnm2 opened this issue Oct 16, 2024 · 16 comments · May be fixed by #76000
Open

Quick info on field exposes generated member name, unlike on accessors #75529

jnm2 opened this issue Oct 16, 2024 · 16 comments · May be fixed by #76000
Assignees
Milestone

Comments

@jnm2
Copy link
Contributor

jnm2 commented Oct 16, 2024

Version Used: 17.12 Preview 3.0

The metadata name of the backing field is being displayed:
Image

But accessors don't show the metadata name of the generated accessor methods:
Image

Perhaps instead, this could show int C.P.field?

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 16, 2024
@RikkiGibson
Copy link
Contributor

Probably the fix belongs in SymbolDisplay?

@KyouyamaKazusa0805
Copy link

Just be curious: what will be displayed in quick info for such backing field? int C.field or just int field?

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 17, 2024

Also:
Image

@RikkiGibson
Copy link
Contributor

Just be curious: what will be displayed in quick info for such backing field? int C.field or just int field?

I think int C.P.field would be best for symbol display. The property is a logical container for the field. @cston in case you have any additional thoughts here.

@RikkiGibson RikkiGibson added Area-Compilers Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it and removed Area-IDE untriaged Issues and PRs which have not yet been triaged by a lead labels Oct 17, 2024
@RikkiGibson RikkiGibson added this to the 17.13 milestone Oct 17, 2024
@RikkiGibson RikkiGibson removed the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label Oct 17, 2024
@RikkiGibson
Copy link
Contributor

Doing a quick check for what additional behaviors need to be specified, using property accessors (e.g. int C.P.get) as a starting point for the way the field name is displayed.

I am not seeing any options or tests which apply customization of how a property accessor name is displayed. I don't think we should offer any option to, for example, omit the field name component from symbol display. But, let's bring the question up with API review.

I am also thinking that the symbol display behavior should be independent of LangVersion. I do not think that SymbolDisplayVisitor.Members has any existing cases of language version affecting the way symbols are displayed.

@RikkiGibson RikkiGibson added the api-ready-for-review API is ready for review, it is NOT ready for implementation label Oct 17, 2024
@RikkiGibson RikkiGibson self-assigned this Oct 17, 2024
@CyrusNajmabadi
Copy link
Member

I'd prefer just naming the property. Is there a reason that would not be suitable?

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 17, 2024

That would be confusing, because the tracked-null state of field and P may differ.

@RikkiGibson
Copy link
Contributor

Agree with @jnm2. The backing field is a distinct symbol, and when using the backing field, it is helpful to be clear that is what you are using and not the containing property.

@CyrusNajmabadi
Copy link
Member

Ah ok. int X.field makes sense. Similar to int X.set.

@jaredpar
Copy link
Member

@RikkiGibson why is this marked as "api-ready-for-review". I don't see any API changes in the comments here.

@RikkiGibson
Copy link
Contributor

It was suggested in offline conversation with @cston that we should run this scenario by API review to decide if we want to introduce any symbol display options related to this feature.

@333fred 333fred removed the api-ready-for-review API is ready for review, it is NOT ready for implementation label Oct 23, 2024
@333fred
Copy link
Member

333fred commented Oct 23, 2024

I think an email discussion with the api review alias, or with the owner of the api (@AlekseyTs and myself) is the appropriate first step here, rather than bringing it to the entire api review meeting.

@RikkiGibson
Copy link
Contributor

We discussed on the internal mailing list and got several folks on board with the suggested design.

  • Display of a backing field symbol will suffix '.field' onto the display of the containing property. This is similar to the way property accessors are handled by suffixing '.get', for example.
  • This only applies when the field itself is used, which generally only occurs within the accessors of the containing property.
  • Therefore display of an example symbol like 'field' in 'int Prop { get => field; }' will result in 'int ContainingType.Prop.field' when the appropriate options are used.
  • No new options are provided for customizing the display. Existing options which affect things like type name and member name qualification continue to be respected.
  • This will apply to all backing field symbols, including those associated with "classic" auto properties. (Such symbols have generally not been displayed in IDE scenarios to date.)
  • Symbol display options which indicate the metadata name should be displayed, should also apply here (e.g. causing the field's unspeakable metadata name to be shown as it is today.)

@jnm2
Copy link
Contributor Author

jnm2 commented Oct 30, 2024

@RikkiGibson Quick question: is the type in string C.P.field going to have the field's nullability annotation?

@RikkiGibson
Copy link
Contributor

For now, I am expecting the field to have, and show, the same type and nullable annotation as the containing property.

If we move forward with null-resilient getters, I am not sure yet whether the inferred nullability should surface in the symbol model. Basically, it seems risky to make it so accessing the TypeWithAnnotations of a symbol (or just the Type in public symbol API), causes a get accessor to be bound and flow analyzed. It could make the access unexpectedly costly, and could introduce cycles which are difficult to stamp out.

Though, FWIW, we know that work is already done and being surfaced in the Quick Info case.

@arunchndr arunchndr modified the milestones: 17.13, 17.13 P2 Nov 13, 2024
@RikkiGibson RikkiGibson linked a pull request Nov 21, 2024 that will close this issue
@RikkiGibson
Copy link
Contributor

It didn't occur to me that changing this behavior for all backing fields would break a billion test baselines, but I will still do it.

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

Successfully merging a pull request may close this issue.

7 participants