feat: resolve custom includes for a program given as a string - #377
feat: resolve custom includes for a program given as a string#377TheGupta2012 wants to merge 1 commit into
Conversation
Argus reviewAuto-review is off for this repo. Tick the box below to run a review on this PR.
Estimated cost
Tip: you can also comment |
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
b58063e to
ea15c60
Compare
load() resolved include statements relative to the file's own path; loads() made no attempt at all, though the two entrypoints are documented as equivalent. A program arriving as a string -- from object storage, or a vendor API -- has no filesystem location to resolve relative includes against, so the failure surfaced downstream as 'Unsupported / undeclared QASM operation', naming the gate rather than the unresolved include. Add an include_dir kwarg naming the directory includes resolve against, and share one include walk between the file and string paths. For load() it is tried before the directory of the including file. Resolution stays opt-in. Without the kwarg loads() reads no files at all and an unresolved include is passed through unchanged, which existing programs rely on; resolving against the working directory by default would instead let program text drive filesystem reads. With the kwarg, an include the directory does not hold raises a ValidationError naming both. Fixes #368 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ea15c60 to
ecfa38e
Compare
Fixes #368.
The problem
load()resolves customincludestatements;loads()silently does not. The two entrypoints are otherwise documented as equivalent, so the asymmetry is easy to hit, and the failure is misleading when you do:The error names the gate, not the unresolved include, which sends you to the gate table rather than to include handling.
Resolution is keyed on a path —
_resolve_include_pathjoinsdirname(base_file)with the include name — and a string has no path. Programs frequently arrive as strings, fetched from object storage or returned by a vendor API, so for those callersload()is not an option and includes could never resolve.The change
One kwarg,
include_dir, naming the directory includes resolve against:preprocess.pykeeps its existing walk;_process_fileis split so the body after the read is reusable for text that came from a string, andinclude_diris threaded through to_resolve_include_pathas one extra candidate, tried first.process_include_statements(filename, include_dir=None)stays signature-compatible.Why not just resolve against the cwd
That is the smaller change — no kwarg at all — and it was considered. Two things stopped it:
_resolve_include_pathdoes not constrain the name, soinclude "../secret.inc";andinclude "/etc/hosts";both resolve. Todayloads(str)never opens a file; defaulting to cwd resolution would change that for every existing caller. Forload()this is fine — the caller handed us a path — but a string carries no such intent. qBraid accepts user-submitted QASM, so this is concrete rather than theoretical.An explicit directory keeps the same mechanism without either property. Resolution stays opt-in: omit the kwarg and
loads()reads no files at all, passing an unresolved include through exactly as before — whichtest_no_include_addedandtest_includes_preservedpin, and which existing programs rely on.Errors
Pass the kwarg and an include the directory does not hold is named, along with the directory — so a typo in the path is self-diagnosing and needs no separate guard:
include_dirwith an already-parsedProgramraisesValueError— it has no include statements left to resolve, so the kwarg cannot do anything, and failing beats silently ignoring it (the principle #356 applied to the other kwargs). A non-string value raisesTypeErrorat the call site.Verification
tox -e format-check: pylint 10.00/10, isort, black, mypy and headers clean.hqslib1.inc(5747 bytes, fromCQCL/tket):loads(bell_state, include_dir=vendor)produces output byte-identical toload()on the same program with the include beside it.One regression was caught by the existing suite during development and fixed: binding
"\n".join(ctx.base_file_header)into the return expression evaluated the header before the walk that appends to it, dropping aqelib1.incdiscovered inside an included file.test_valid_include_processing[include_qasm2_backward.qasm]failed on it.Tests added (
tests/test_include.py)test_loads_resolves_include_from_include_dirtest_loads_and_load_agree_on_the_same_programtest_nested_include_resolves_beside_the_file_that_named_ittest_loads_reports_the_unresolved_include_by_nametest_loads_without_include_dir_still_passes_includes_throughtest_include_dir_wins_over_the_directory_of_the_fileload()test_include_dir_rejected_for_a_parsed_programProgramcombination raisestest_include_dir_rejects_a_non_pathFollow-ups not taken
include_path=[...](several directories) and per-file overrides are both additive on top of this and can follow if there is demand.Related
#370 / #378 is the other half of loading a Quantinuum compiled program from a string: this PR locates
hqslib1.inc, and #378 lets itsopaquelines parse. #378 is stacked on this branch.