quil-lang / quilc

The optimizing Quil compiler.
Apache License 2.0
454 stars 73 forks source link

Add compiler check that warns/errors on multiple defgates of the same name #797

Closed Spin1Half closed 2 years ago

Spin1Half commented 2 years ago

As of right now, having multiple defined gates of the same name is considered undefined behavior by the quil lang spec. It might be nice for quilc to have a quick check that verifies no duplicate names exist in the gate definitions of a parsed program object.

Remark: care should probably be taken to make this check take O(n) time in the number of gate defs as opposed to O(n^2) [although the O(n^2) approach allow the check to be in place while the O(n) approach requires building a hashset].

karlosz commented 2 years ago

If you're wary about building a hash set, you could just use symbol property lists instead. E.g.

(setf (get <name> 'gate-definition) <some true value>)

and then (get <name> 'gate-definition) => <some true value>.

I don't know how Lispers feel about symbol property lists these days though. It's a bit esoteric.

Edit: You should definitely just use a hash set instead.

stylewarning commented 2 years ago

Note that ambiguous definitions due to name conflicts are checked here. An ambiguous-definition-condition is raised.

This is later handled in a handler-bind and is controlled by the ambiguous-definition-handler argument (a function) in %parse-quil and parse-quil functions.

It's a mistake that this argument is not documented clearly, and there are no obvious options one can provide it. Currently it just "continues" (i.e., ignores the error without warning), but was clearly put here to anticipate a more descriptive action.

Side note, it will error if there are conflicts in defined memory. You can see this here in a "functional disjunction" layering memory error handling on top of the user-supplied handler. Note the comment here. You can see how a memory error is actually handled here.

The bottom line is this:

We might also consider a PRAGMA to allow the latest gate definition be the dominant one, thus allowing gates to be easily redefined for testing purposes.

As @Spin1Half said, the original Quil paper, last sentence of section III.D, says conflicting definitions is undefined behavior. So that's the status quo.