Fix repeated Coqargs parsing with ~init - #22334
Open
finalchild wants to merge 1 commit into
Open
Conversation
finalchild
force-pushed
the
agent/coqargs-compositional-parsing
branch
5 times, most recently
from
August 8, 2026 17:22
4eb43a1 to
bbb67a8
Compare
finalchild
marked this pull request as ready for review
August 8, 2026 17:43
Coqargs.parse_args returns ordered list fields, but a returned value reused as init was treated as a reverse-order accumulator. This could reorder existing options. Package declarations were also resolved on every parse, potentially inserting derived load paths repeatedly. Normalize init before entering the parser and normalize the result afterward. Keep package declarations unresolved until init_document so package load paths are installed once at their point of use.
finalchild
force-pushed
the
agent/coqargs-compositional-parsing
branch
from
August 8, 2026 17:45
bbb67a8 to
29ae900
Compare
Contributor
|
see also #22259 |
Author
I'll rebase on it once it's merged. This PR can then focus on fixing the reversal. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Coqargs.parse_argsaccepts an existing option value through~init(also exposed asCoqinit.parse_arguments ~initial_args). A value returned by one call should therefore be validinput to a later call. Currently it is not.
The parser builds ordered list fields by prepending, and the public wrapper reverses those fields
before returning them. On a later call, the already-normalized
initvalue is treated as if itwere still in the parser's reverse-accumulator representation. The wrapper then reverses the
whole result again. Consequently, even parsing an empty argument list can reorder options that
were already present.
-packagehas an additional problem: every parse resolves the complete accumulated package listand inserts the resulting load paths into
vo_includes. Calling the parser again can thereforeresolve old packages again and mix duplicate derived paths with explicit
-Q/-Rpaths.This is a correctness bug in the existing
~initAPI, rather than an API refactoring.Impact
Load-path order determines precedence when physical paths or logical prefixes overlap. Clients
that parse arguments in layers can therefore resolve a
.vofile through a different mappingthan a one-pass command-line invocation.
VsRocq does this when it combines project and process/document arguments. With Dune projects with (generate_project_file),
reordering the project-root mapping and the
_build/defaultmapping can make Rocq assign the wronglogical name to a compiled file, producing errors of the form:
Other embedders using
Coqargs.parse_args ~initorCoqinit.parse_arguments ~initial_argsare affected by the same ordering and repeated-resolutionbehavior.
Fix
parse_argsboundary as normalized, in correct declaration order. Thus, reverse the lists also in the beginning, not only in the end.pre.packageswhile parsing. Resolve them once ininit_document, atthe point where package load paths are installed.
rocq-runtime.libdependency from thecoqargslibrary tosysinit, following themove of
Rocq_package.resolvecall.Resolving only packages newly seen by each parser call was considered, but would leave parsing
dependent on Findlib state and require tracking which package closures had already been expanded.
Deferring resolution keeps parsing repeatable and gives package expansion a single owner.
Compatibility
There is no type or record-shape change. The intended behavior changes are observable, however:
pre.packages, are now consistently returned in declaration order (corrected behavior);Coqargs.parse_args,pre.vo_includescontains only explicit-Q/-Rentries, not paths derived from
-package(field semantic changes);init_documentrather than by argument parsing.The normal
Coqinitstartup path already initializes the document, so it receives the samepackage load paths at the correct stage. A downstream caller that uses
Coqargs.parse_argsdirectly and expects package-derived entries in
pre.vo_includesbefore document initializationwill need to adjust to the new resolution point. This is the principal downstream compatibility
risk.
Tests
coqargsunit test compares one-pass parsing with two calls joined through~init, checksthat an empty incremental parse is an identity, verifies declaration order for all accumulator
fields, and verifies that parsing does not resolve a nonexistent package.
misc/rocq-find.shnow exercises the new resolution point end to end: a package and itstransitive dependency are compiled and loaded through
-package, and a missing package muststill fail during document initialization.
AI disclaimer
AI debugged the issue, human discussed the fix direction with AI, AI wrote the code and PR, and human reviewed them. I'm not good at shell scripts, so please check whether the test script is good enough.