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 the implementation of TypeVariable.hashCode() #260

Merged
merged 1 commit into from
Sep 15, 2022

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Sep 15, 2022

Previous implementation of this method was wrong in case the type variable has an implicit bound of Object. To conserve memory, TypeVariable stores the information whether the first bound is implicitly Object as the most significant bit of the hash field, which also serves as a hash code cache.

Specifically, if the type variable has an implicit Object bound, the MSB is set to 1, otherwise it is set to 0. When it's 1, the hashCode() method used to return one value when first called and a different value for subsequent calls.

This is because the beginning of hashCode() looks like this:

int hash = this.hash & HASH_MASK;
if (hash != 0) {
    return hash;
}

When the hash code has already been computed, the method returns the stored value, properly masked to always have a 0 at the MSB. When the hash code has not been computed yet, it is computed, cached and returned. The code for caching the computed value and returning it used to look like this:

return this.hash |= (hash & HASH_MASK);

That is partly wrong. The hash code is cached correctly, but then the whole value of this.hash is returned, including the non-masked MSB, which is 1 when the first bound is implicitly Object.

This commit splits the two operations (storing into cache and returning) so that the hashCode() method always returns a properly masked value:

hash &= HASH_MASK;
this.hash |= hash;
return hash;

Previous implementation of this method was wrong in case the type variable
has an implicit bound of `Object`. To conserve memory, `TypeVariable` stores
the information whether the first bound is implicitly `Object` as the most
significant bit of the `hash` field, which also serves as a hash code cache.

Specifically, if the type variable has an implicit `Object` bound, the MSB
is set to `1`, otherwise it is set to `0`. When it's `1`, the `hashCode()`
method used to return one value when first called and a different value for
subsequent calls.

This is because the beginning of `hashCode()` looks like this:

    int hash = this.hash & HASH_MASK;
    if (hash != 0) {
        return hash;
    }

When the hash code has already been computed, the method returns the stored
value, properly masked to always have a `0` at the MSB. When the hash code
has not been computed yet, it is computed, cached and returned. The code
for caching the computed value and returning it used to look like this:

    return this.hash |= (hash & HASH_MASK);

That is partly wrong. The hash code is cached correctly, but then the whole
value of `this.hash` is returned, including the non-masked MSB, which is `1`
when the first bound is implicitly `Object`.

This commit splits the two operations (storing into cache and returning)
so that the `hashCode()` method always returns a properly masked value:

    hash &= HASH_MASK;
    this.hash |= hash;
    return hash;
@Ladicek Ladicek added this to the 3.0.1 milestone Sep 15, 2022
@Ladicek
Copy link
Contributor Author

Ladicek commented Sep 15, 2022

For historical record, this bug was introduced in this commit: 9133038

@Ladicek Ladicek merged commit a174a3f into smallrye:main Sep 15, 2022
@Ladicek Ladicek deleted the fix-typevariable-hash-code branch September 15, 2022 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant