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

Fix System.Text.Json source code annotation RequiresUnreferencedCodeAttribute by moving it to class level #67536

Closed
LakshanF opened this issue Apr 4, 2022 · 4 comments
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework
Milestone

Comments

@LakshanF
Copy link
Member

LakshanF commented Apr 4, 2022

Description

As mentioned in this comment, there is a desire to revisit some source in the annotation in System.Text.Json namespace to hoist it to class level once issue is fixed. Its done now.

Reproduction Steps

No observable behavioral change with the issue but only impacts code hygiene

Expected behavior

RequiresUnreferencedCodeAttribute to be moved up to the class header

Actual behavior

RequiresUnreferencedCodeAttribute is at many members in the class

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Apr 4, 2022
@ghost
Copy link

ghost commented Apr 4, 2022

Tagging subscribers to this area: @dotnet/area-system-text-json, @gregsdennis
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

As mentioned in this comment, there is a desire to revisit some source in the annotation in System.Text.Json namespace to hoist it to class level once issue is fixed. Its done now.

Reproduction Steps

No observable behavioral change with the issue but only impacts code hygiene

Expected behavior

RequiresUnreferencedCodeAttribute to be moved up to the class header

Actual behavior

RequiresUnreferencedCodeAttribute is at many members in the class

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: LakshanF
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@krwq krwq added enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework and removed untriaged New issue has not been triaged by the area owner labels Jun 7, 2022
@ghost
Copy link

ghost commented Jun 7, 2022

Tagging subscribers to 'linkable-framework': @eerhardt, @vitek-karas, @LakshanF, @sbomer, @joperezr
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

As mentioned in this comment, there is a desire to revisit some source in the annotation in System.Text.Json namespace to hoist it to class level once issue is fixed. Its done now.

Reproduction Steps

No observable behavioral change with the issue but only impacts code hygiene

Expected behavior

RequiresUnreferencedCodeAttribute to be moved up to the class header

Actual behavior

RequiresUnreferencedCodeAttribute is at many members in the class

Regression?

No

Known Workarounds

No response

Configuration

No response

Other information

No response

Author: LakshanF
Assignees: -
Labels:

enhancement, area-System.Text.Json, untriaged, linkable-framework

Milestone: -

@krwq krwq added this to the Future milestone Jun 7, 2022
@eiriktsarpalis
Copy link
Member

I previously attempted making that change, but it turned out that adding RUC annotations on the type level introduced other complications, for example it was triggering linker warnings when consuming const or static members on that class even though it shouldn't.

Keeping the annotations on the constructors should be fine until annotation issues have been ironed out, so I don't think we need to track this via an open issue. I'm going to close this.

@vitek-karas
Copy link
Member

for example it was triggering linker warnings when consuming const or static members on that class even though it shouldn't.

This is by design - putting the RUC on class means that everything about the class is dangerous. It is functionally equivalent to

  • putting RUC on all .ctors
  • putting RUC on all static methods and fields

If the class is only dangerous when instantiated, then you should put RUC on .ctors alone.

@ghost ghost locked as resolved and limited conversation to collaborators Oct 2, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json enhancement Product code improvement that does NOT require public API changes/additions linkable-framework Issues associated with delivering a linker friendly framework
Projects
None yet
Development

No branches or pull requests

4 participants