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: Relax Interner trait bounds for borrowed elements #14039

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

raynelfss
Copy link
Contributor

Summary

Fix trait bounds in certain methods of Interner to allow for the use of elements that satisfy the trait bounds of Equivalent<T> in IndexMap.

Details and comments

In a very special edge case for the Interner a user was not able to check if it contained an instance of String from an &str. The following commits relax the trait bounds on the contains, try_key, and insert methods, to allow for more flexibility by behaving similarly to the trait Equivalent<T> from IndexMap.

- Fix trait bounds in certain methods of `Interner` to allow use of elements that satisfy the trait bounds of `Equivalent<T>` in `IndexMap`.
@raynelfss raynelfss added the Changelog: None Do not include in changelog label Mar 17, 2025
@raynelfss raynelfss requested a review from a team as a code owner March 17, 2025 17:46
@qiskit-bot
Copy link
Collaborator

One or more of the following people are relevant to this code:

  • @Qiskit/terra-core

@coveralls
Copy link

coveralls commented Mar 17, 2025

Pull Request Test Coverage Report for Build 13931421405

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 5 of 15 (33.33%) changed or added relevant lines in 1 file are covered.
  • 50 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.02%) to 88.07%

Changes Missing Coverage Covered Lines Changed/Added Lines %
crates/circuit/src/interner.rs 5 15 33.33%
Files with Coverage Reduction New Missed Lines %
crates/accelerate/src/unitary_synthesis.rs 1 94.69%
crates/qasm2/src/lex.rs 4 93.23%
qiskit/transpiler/passes/scheduling/time_unit_conversion.py 9 92.31%
crates/qasm2/src/parse.rs 12 97.15%
crates/circuit/src/bit.rs 24 87.14%
Totals Coverage Status
Change from base Build 13905217637: -0.02%
Covered Lines: 72612
Relevant Lines: 82448

💛 - Coveralls

pub fn try_key<Q>(&self, value: &Q) -> Option<Interned<T>>
where
Q: ?Sized + Hash + Eq + ToOwned,
T: Borrow<Q> + ToOwned<Owned = <Q as ToOwned>::Owned>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can instead reexport indexmap's Equivalent with pub use indexmap::Equivalent in this file? And then just use it in these method bounds so it's easier to understand what's happening.

Copy link
Contributor Author

@raynelfss raynelfss Mar 17, 2025

Choose a reason for hiding this comment

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

I tried putting in Q: Equivalent<T> alone, but then the compiler will complain because, to satisfy the trait bounds of Q: Equivalent<T>, Q needs to be: Q: ?Sized + ToOwned, and T needs to satisfy T: Borrow<Q> as a minimum. The rest is more specificity to mention that the owned versions of both T and Q need to be of the same base type.

Bringing those trait bounds in already makes using Q: Equivalent<T> redundant, but it is implicitly satisfied. Is there an easier wa of conveying this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this would work?

pub fn try_key<Q>(&self, value: &Q) -> Option<Interned<T>>
    where
        Q: ?Sized + Hash + Equivalent<T::Owned>,
    {
        todo!()
    }

Copy link
Contributor Author

@raynelfss raynelfss Mar 18, 2025

Choose a reason for hiding this comment

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

This works as long as we also specify that T: Borrow<Q>. Unfortunately, though, it's still not enough for the insert method. Fixed in d5161e3!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changelog: None Do not include in changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants