Only warn on duplicate members within a single descriptors file by jtschuster · Pull Request #101574 · dotnet/runtime

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

jtschuster · 2024-04-25T21:24:46Z

In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we should keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

dotnet-policy-service · 2024-04-25T21:25:16Z

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

MichalStrehovsky · 2024-04-26T06:56:24Z

This looks good, but I always wondered why we need to warn.

Could this warn in legitimate situations, like embedded descriptors in two different XML files preserving the same thing, or even within the same descriptor if the preservations are under different feature flags and we happen to enable both feature flags at the same time, leading to both being used?

jtschuster · 2024-04-26T17:18:15Z

This shouldn't warn on the same member being marked in different embedded XML files, a new object is created for each descriptor file with a new _preservedMembers set. However, I think it would warn if the member is listed twice for different feature flags and both are enabled.

I'm fine removing the warnings, I don't see a lot of value in them.

sbomer · 2024-04-26T18:32:27Z

Looks like it was introduced as a convenience to help clean up some descriptor files in dotnet/linker#293. I'm fine with removing them now that we've added feature switches. Another option is to make it an info message instead.

…escriptionDuplicatePreserve

sbomer

LGTM, thank you!

…gle descriptors file (dotnet#101574) In the trimmer we rely on the order of marking to determine if a descriptors file has a duplicate member. As we transition to the dependency analysis framework, the marking order changes, and this exposed scenarios where a descriptors file is found after some items have already been marked. This caused unnecessary IL2025 warnings (Duplicate preserve in descriptor file) to be reported. Instead of using the global marking state, we will keep a per-descriptor-file set of which members are preserved and only report duplicates within that set.

Only warn on duplicate members within a single descriptors file
afedfb7

jtschuster requested review from MichalStrehovsky, sbomer and vitek-karas April 25, 2024 21:24

ghost added the area-Tools-ILLink .NET linker development as well as trimming analyzers label Apr 25, 2024

dotnet-policy-serviceBot added the linkable-framework Issues associated with delivering a linker friendly framework label Apr 25, 2024

dotnet-policy-serviceBot assigned jtschuster Apr 25, 2024

jtschuster mentioned this pull request Apr 25, 2024
Linker dependency framework #101277
Merged

jtschuster marked this pull request as ready for review April 25, 2024 23:09

jtschuster requested a review from marek-safar as a code owner April 25, 2024 23:09

This was referenced Apr 26, 2024
WasmTestOnChrome-System.Runtime.Tests.WorkItemExecution fails with exit code 3221226505 #101464
Closed
[browser] Stack overflow in linking with emcc #101467
Closed
Dead lettering tests #101524
Closed
[WASI][AOT] LLVM ERROR: out of memory #101533
Closed

Make duplicate preserve an info message
a600348

build-analysisBot mentioned this pull request Apr 26, 2024
[wasm][firefox] crit: OpenQA.Selenium.WebDriverException: Failed to decode response from marionette #101617
Open
3 tasks

jtschuster added 3 commits April 26, 2024 15:26

Merge branch 'main' ofhttps://github.com/dotnet/runtimeinto ILLinkD…
a0ea640
…escriptionDuplicatePreserve

Update expectations in xml warnings tests
b27bc04

Make method internal for API compat
1abe4b3

build-analysisBot mentioned this pull request Apr 27, 2024
AbbreviatedMonthGenitiveNames_Get_ReturnsExpected_HybridGlobalization failing with string mismatch #101634
Closed

sbomer approved these changes Apr 29, 2024
View reviewed changes

jtschuster merged commit 62e7396 into dotnet:mainApr 29, 2024

github-actionsBot locked and limited conversation to collaborators May 31, 2024