You've got a compilation error because this needs to be:

You can test these ahead of time by running

cargo make test-all

Along the same lines, you need to regenerate the type stubs and check them, since you've made changes to the Python API. You can do that with:

cargo make generate-stubs

Since that requires the stubs feature, in this instance, it would have caught the issue.

Since you're not able to use pickleable_new here¹, you should implement __getnewargs__ manually so that CircuitDefinitions (and its subclasses) remain pickleable.

Since doing so is purely for Python users, our convention is to put it in a quilpy module. In this case, the appropriate place is src/instructions/quilpy.rs, likely around line 460.

Here's one possible implementation (the important thing is that its arguments are compatible with the method marked #[new]):

#[cfg_attr(feature = "stubs", gen_stub_pymethods)]
#[pymethods]
impl CircuitDefinition {
    fn __getnewargs__<'py>(&self, py: pyo3::Python<'py>) -> PyResult<Bound<'py, PyTuple>> {
        (
            self.name.clone(),
            self.parameters.clone(),
            self.qubits.clone(),
            self.instructions.clone(),
        ).into_pyobject(py)
    }
}

You probably don't want set, if these must be Qubit::Variable, as otherwise Python users can set the list to other kinds of Qubits.

To that end, I think you should at least document that this must be Qubit::Variable explicitly. You could enforce that with a newtype that wraps String and serves as the type of both Qubit::Variable and the type of this Vec, but its not clear to me that's any better than just using String itself, which you're trying to change. That leads me to the obvious question:

Why should this type be Vec<Qubit> if it really is restricted to only the Variable variant?

The main reason I had to make it Vec<Qubit> instead of the prior Vec<String> is because I needed to extract &Qubits from the circuit definition.

I could try to make something like this work?

enum QubitRef<'a> {
    Fixed(u64),
    Placeholder(QubitPlaceholder),
    Variable(&str),
}

Nit: I think you should just pull the try_new method into a separate impl block instead of repeating the #[cfg(feature = "python")]s.

As I look at this implementation, I am starting to think we should have an actual QubitIterator that can encapsulate this logic without all the collects. I'd like to see some performance details on this, as perhaps it wouldn't matter anyway, but at the very least, the implementation gets pretty messy here. I could imagine it being more of a performance/usability improvement for the Python API, but again, we'd need some profiling data to really say.

Alternatively, I could just force it to be HashSet or Vec. It was Vec previously, but it makes more sense (imo) to be HashSet -- all the consumers either didn't care or would have preferred HashSet.

This can result in the same used_qubits as the old add_instruction implementation: if you call resolve_placeholders_with_custom_resolvers() (which calls this method) on a Program with DEFCAL instructions, the Variables end up in the cache. That was also true before this PR, but it represents an inconsistency between the way add_instruction and this method work within this PR.

I'll also note that DEFCIRCUIT qubits will also make their way into the cache after calling this method, but that wasn't true before the PR.