Skip to content

Suppress IDE0060 breaking local builds#119772

Merged
kg merged 2 commits into
dotnet:mainfrom
kg:remove-unused-parameter-seed
Sep 16, 2025
Merged

Suppress IDE0060 breaking local builds#119772
kg merged 2 commits into
dotnet:mainfrom
kg:remove-unused-parameter-seed

Conversation

@kg

@kg kg commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Not sure why this unused parameter is here, though. Looks like a bug that it's not used?

Copilot AI review requested due to automatic review settings September 16, 2025 16:27
@github-actions github-actions Bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Sep 16, 2025

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull Request Overview

This PR suppresses an IDE0060 warning (unused parameter) for the seed parameter in the HashCodeBuilder constructor. The author notes this appears to be a bug since the parameter is not used in the implementation.

Key Changes

  • Adds pragma directives to suppress IDE0060 warning around the constructor
  • No functional code changes

Comment thread src/libraries/Common/src/Internal/VersionResilientHashCode.cs Outdated
@kg kg requested a review from MichalStrehovsky September 16, 2025 16:27
@xtqqczze

xtqqczze commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Not sure why this unused parameter is here

Introduced in #119385, replacing the previous implementation:

public HashCodeBuilder(string seed)
{
    _hash1 = 0x6DA3B944;
    _hash2 = 0;
    _numCharactersHashed = 0;

    Append(seed);
}

cc: @MichalStrehovsky

@tarekgh

tarekgh commented Sep 16, 2025

Copy link
Copy Markdown
Member

@jkotas can this get merged? It looks like it's causing a lot of failures in CI. @MichalStrehovsky looks offline.

@kg

kg commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

I'm open to also adding the missing Append() call since it looks like that's the correct fix.

@xtqqczze

Copy link
Copy Markdown
Contributor

I'm open to also adding the missing Append() call since it looks like that's the correct fix.

It does look like the correct fix. As far as I can tell, it doesn’t currently impact behavior, since the seed span is empty at all of the call sites.

@tarekgh

tarekgh commented Sep 16, 2025

Copy link
Copy Markdown
Member

Maybe we can add it under a condition checking the seed is not empty? or if Append(seed); is not affected by empty span, then we can call it anyway in case in the future we can get non-empty span.

@xtqqczze

Copy link
Copy Markdown
Contributor

Maybe we can add it under a condition checking the seed is not empty? or if Append(seed); is not affected by empty span, then we can call it anyway in case in the future we can get non-empty span.

It makes no difference to behavior:

public void Append(ReadOnlySpan<byte> src)
{
if (src.Length == 0)
return;

@tarekgh

tarekgh commented Sep 16, 2025

Copy link
Copy Markdown
Member

Let's call Append anyway then.

@kg

kg commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

Let's call Append anyway then.

Done

@kg

kg commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

Should I ba-g to unblock people? I verified it builds locally but haven't run tests.

@tarekgh tarekgh added area-NativeAOT-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Sep 16, 2025
@tarekgh tarekgh added this to the 11.0.0 milestone Sep 16, 2025
@dotnet-policy-service

Copy link
Copy Markdown
Contributor

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

@xtqqczze

xtqqczze commented Sep 16, 2025

Copy link
Copy Markdown
Contributor

Should I ba-g to unblock people? I verified it builds locally but haven't run tests.

All the call sites are inside an internal class. I think there is fairly obviously no change in behavior, see grep output for HashCodeBuilder:

src/coreclr/tools/Common/Internal/Metadata/NativeFormat/MetadataTypeHashingAlgorithms.cs
6:using HashCodeBuilder = Internal.VersionResilientHashCode.HashCodeBuilder;
14:        private static void AppendNamespaceHashCode(ref HashCodeBuilder builder, NamespaceDefinitionHandle namespaceDefHandle, MetadataReader reader, bool appendDot)
35:        private static void AppendNamespaceHashCode(ref HashCodeBuilder builder, NamespaceReferenceHandle namespaceRefHandle, MetadataReader reader, bool appendDot)
60:            HashCodeBuilder builder = new HashCodeBuilder(""u8);
82:            HashCodeBuilder builder = new HashCodeBuilder(""u8);

src/libraries/Common/src/Internal/VersionResilientHashCode.cs
17:        public struct HashCodeBuilder
23:            public HashCodeBuilder(ReadOnlySpan<byte> seed)

@kg

kg commented Sep 16, 2025

Copy link
Copy Markdown
Contributor Author

/ba-g Unblocking CI, change is behavior-preserving

@kg kg merged commit 579bd79 into dotnet:main Sep 16, 2025
33 of 79 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Oct 17, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants