Skip to content

Conversation

@sbomer
Copy link
Member

@sbomer sbomer commented Sep 14, 2023

This tracks whether properties have been marked yet, and avoids marking them multiple times. Fixes a recursion issue where marking a property can cause marking of attributes which may cause marking of the same property.

I removed the AlreadyMarked assert because #84620 propagates the Kind from methods to properties - so it can look as if a property is being marked from a descriptor the first time it is encountered.

Fixes #92064

Fixes the illink part of #83581. That issue mentions a problem with the analyzer behavior, but the testcase I'm fixing in this PR doesn't seem to show duplicate warnings from the analyzer. Can we consider #83581 fixed by this change?

@sbomer sbomer requested a review from vitek-karas September 14, 2023 22:41
@sbomer sbomer requested a review from marek-safar as a code owner September 14, 2023 22:41
@ghost ghost added area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework labels Sep 14, 2023
@ghost ghost assigned sbomer Sep 14, 2023
@ghost
Copy link

ghost commented Sep 14, 2023

Tagging subscribers to this area: @agocke, @sbomer, @vitek-karas
See info in area-owners.md if you want to be subscribed.

Issue Details

This tracks whether properties have been marked yet, and avoids marking them multiple times. Fixes a recursion issue where marking a property can cause marking of attributes which may cause marking of the same property.

I removed the AlreadyMarked assert because #84620 propagates the Kind from methods to properties - so it can look as if a property is being marked from a descriptor the first time it is encountered.

Fixes #92064

Fixes the illink part of #84620. That issue mentions a problem with the analyzer behavior, but the testcase I'm fixing in this PR doesn't seem to show duplicate warnings from the analyzer. Can we consider #83581 fixed by this change?

Author: sbomer
Assignees: -
Labels:

linkable-framework, area-Tools-ILLink

Milestone: -

@sbomer
Copy link
Member Author

sbomer commented Sep 15, 2023

@vitek-karas I realized the same bug exists for events so I fixed that the same way - PTAL.

@sbomer sbomer changed the title Avoid marking property attributes multiple times Avoid marking property/event attributes multiple times Sep 15, 2023
Copy link
Member

@vitek-karas vitek-karas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice

NativeAot requires a call to GetCustomAttribute for the
attributes to be kept.
@sbomer sbomer merged commit 65216da into dotnet:main Sep 15, 2023
@sbomer
Copy link
Member Author

sbomer commented Sep 15, 2023

/backport to release/8.0

@github-actions
Copy link
Contributor

Started backporting to release/8.0: https://github.com/dotnet/runtime/actions/runs/6202438968

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dotnet publish command fails with error NETSDK1144: Optimizing assemblies for size failed

2 participants