hammer with given lemmas#212
Conversation
There was a problem hiding this comment.
4 issues found across 4 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/plugin/features.mli">
<violation number="1" location="src/plugin/features.mli:14">
P3: The extract_given_lemmas return comment says it is a temporary file name, but the function actually returns an hhdef list of predicted dependencies. The interface contract is misleading.</violation>
</file>
<file name="src/plugin/features.ml">
<violation number="1" location="src/plugin/features.ml:230">
P2: extract_given_lemmas can throw a misleading "Predictor did not return advice." error when ths_deps is empty because it reads from an empty temp file. This is likely for empty user-provided lemmas and should return an empty selection instead of crashing.</violation>
</file>
<file name="src/plugin/hammer_main.ml">
<violation number="1" location="src/plugin/hammer_main.ml:263">
P2: Catch-all exception handler in get_argus will swallow Sys.Break interrupts (Ctrl+C), preventing users from interrupting a hanging tactic; re-raise Sys.Break or catch specific exceptions.</violation>
<violation number="2" location="src/plugin/hammer_main.ml:734">
P2: do_extraction duplicates the bulk of do_predict’s parallel execution, cleanup, and minimization logic. This makes maintenance error-prone because changes to prover orchestration must be kept in sync across two functions.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| | [ "hammer" "[" constr_list(l) "]" ] -> { hammer_tac l } | ||
| END | ||
|
|
||
|
|
There was a problem hiding this comment.
I will remove the whitespace-only changes
| deps | ||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
I will remove the whitespace-only changes
| (Str.split (Str.regexp " ") | ||
| (try input_line ic with End_of_file -> | ||
| close_in ic; Sys.remove oname; | ||
| raise (HammerError "Predictor did not return advice."))) |
There was a problem hiding this comment.
The error message is misleading because there is no prediction going on here (we supply the lemmas, not predict them)
There was a problem hiding this comment.
This part of the code is redundant. I will remove it.
|
Thank you for the contribution, but I don't quite see the point. If we know which lemmas we want to use, we can just use the |
|
Hello, my idea is to make hammer easier to integrate with other automated tools. For example, if a tool implements a new lemma retrieval algorithm, this syntax would allow integration with hammer without requiring modifications to hammer's internal algorithms. This new syntax could serve as a general-purpose interface between Rocq and ATPs and is better support for non-interactive, programmatic usage. |
Yes, this makes sense if by lemma retrieval you mean quickly preselecting a large number of possibly relevant lemmas. The whole ATP backend of Hammer is nothing more than a lemma retrieval algorithm itself. So by providing lemmas to the The only thing that ATPs do in a hammer is relevant lemma selection. The proof found by the ATPs is discarded. If you don't need further relevant lemma selection, you should feed your retrieved lemmas to If you have a tool that pre-selects a large number of lemmas (essentially a replacement for hammer's If your lemma retrieval tool is precise enough to select e.g. around 10 relevant lemmas, then there is little point in feeding them further to the ATPs - you should feed them to I think it makes sense to include this PR, but the use-cases seem to be rare. Also, the issues with the PR code need to be addressed. |
|
|
||
| TACTIC EXTEND Hammer_tac | ||
| | [ "hammer" ] -> { hammer_tac () } | ||
| | [ "hammer" ] -> { hammer_tac [] } |
There was a problem hiding this comment.
Don't use empty list to indicate no extraction. Create a datatype with constructors for extraction and prediction.
There was a problem hiding this comment.
Also, the "lemma extraction" should be called something else to avoid confusion with feature extraction. Perhaps "lemma choice"?
There was a problem hiding this comment.
I create a datatype hammer_mode for prediction and choice:
type hammer_mode =
| Prediction
| Choice of EConstr.t listCo-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
| let goal_deps = List.filter (fun a -> Hhlib.StringSet.mem a names) goal_deps in | ||
| let ths_deps = List.sort_uniq Stdlib.compare (goal_deps @ (List.concat (List.map write_def lems))) in | ||
|
|
||
| let oname = Filename.temp_file ("coqhammer_out" ^ atpname) "" in |
There was a problem hiding this comment.
I don't understand why we're writing to files here at all. We should just convert the lemmas to hhdefs without going through the type system. The only reason extract creates files is to interface with the separate predict program.
There was a problem hiding this comment.
This is intended to reuse part of the previous code. Indeed, writing to a file is unnecessary here. I will remove this redundant code.
| if !Opt.debug_mode then | ||
| Msg.info ("After filtering: " ^ string_of_int (List.length defss) ^ " Coq objects."); | ||
| let names = Hhlib.strset_from_lst (List.map get_hhdef_name defss) in | ||
| let write_def def = |
There was a problem hiding this comment.
write_def is a confusing name for a function that doesn't do any IO
| fname | ||
|
|
||
| let extract_given_lemmas (hyps : hhdef list) (defs : hhdef list) (lems : hhdef list) (goal : hhdef) (atpname : string) = | ||
| Msg.info "Extracting definitions..."; |
There was a problem hiding this comment.
"extract" and "extracting" is poor naming, because it suggest connection to feature extraction and we're not doing that here. Perhaps "choice" and "choosing"?. Maybe this function should not be in the features.ml file, but a separate one?
There was a problem hiding this comment.
Yes, choose_given_lemmas might be a better name. I think we don't need a separate file, as this functionality is quite similar to the extract function in features.ml. Although we don't need to use features for prediction, the functions used are very similar. Having a separate file just for a single function seems a bit redundant.
|
Thank you for your suggestion. I will address the issues in the code. |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/plugin/features.ml">
<violation number="1" location="src/plugin/features.ml:200">
P2: Empty lemma lists are now treated as an error, but review feedback requires graceful handling of empty lists. This rejects valid inputs like `hammer []` and violates the stated requirement.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if lems = [] then | ||
| raise (HammerError ("No lemmas given for choice.")); |
There was a problem hiding this comment.
P2: Empty lemma lists are now treated as an error, but review feedback requires graceful handling of empty lists. This rejects valid inputs like hammer [] and violates the stated requirement.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/plugin/features.ml, line 200:
<comment>Empty lemma lists are now treated as an error, but review feedback requires graceful handling of empty lists. This rejects valid inputs like `hammer []` and violates the stated requirement.</comment>
<file context>
@@ -196,46 +194,27 @@ let extract (hyps : hhdef list) (defs : hhdef list) (goal : hhdef) : string =
+let choose_given_lemmas (hyps : hhdef list) (defs : hhdef list) (lems : hhdef list) (goal : hhdef) (atpname : string) =
+ Msg.info "Choosing definitions...";
let defss = List.filter is_nontrivial defs in
+ if lems = [] then
+ raise (HammerError ("No lemmas given for choice."));
if !Opt.debug_mode then
</file context>
| if lems = [] then | |
| raise (HammerError ("No lemmas given for choice.")); | |
| if lems = [] then | |
| Msg.info "No lemmas given for choice."; |
Hello, CoqHammer is an excellent automated theorem proving tool that I have been using on Rocq. However, I notice that the current implementation only selects lemmas through clustering algorithms (by using
predictcommand). This modification extends hammer to support user-specified lemmas.I add support for an optional syntax to provide specific lemmas to the
hammertactic:The new syntax,
hammer [lemma1 lemma2 ...]., allows users to explicitly specify which lemmas to use. I think this makes hammer more flexible.Summary by cubic
Add optional lemma selection to hammer so users can guide premise choice. Default hammer behavior stays the same.
Written for commit 03f915e. Summary will update on new commits.