-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add hypothesis and dv usage qualifiers to VERIFY PROOF #92
base: master
Are you sure you want to change the base?
Conversation
Can look into this, but probably not before weekend. |
@@ -11,14 +11,24 @@ | |||
|
|||
#include "mmdata.h" | |||
|
|||
char verifyProof(long statemNum); | |||
/* Flags for verifyProof */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When declaring new constants, comment their semantics at this point. All development environments I know of will automatically look up that lines and present an excerpt to the developer on request.
src/mmveri.h
Outdated
long statementNum, long step, flag unkHypFlag); | ||
long statementNum, long step, flag unkHypFlag, | ||
/* For checking for unused $d conditions: */ | ||
flag verifyFlag, nmbrString *djRVar, nmbrString *djOVar); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A comment starting with /*! is a typical doxygen command. Try to follow that scheme and use /param to explain the parameters you introduced. How does your addition affect the error behaviour? Will it throw exceptions not seen before? Can you establish pre- / post-conditions or invariants with respect to your added code? Use doxygen /pre /post and /invariant tags to describe these and allow test code designers to write a test suite later.
src/mmveri.c
Outdated
@@ -18,11 +18,16 @@ struct getStep_struct getStep = {0, 0, 0, 0, 0, | |||
NULL_PNTRSTRING}; | |||
|
|||
/* Verify proof of one statement in source file. Uses wrkProof structure. | |||
Assumes that parseProof() has just been called for this statement. */ | |||
Assumes that parseProof() has just been called for this statement. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest to convert this into a doxygen style comment starting with /*! . Use tags to categorize the individual portions of the comment.
src/mmveri.c
Outdated
@@ -18,11 +18,16 @@ struct getStep_struct getStep = {0, 0, 0, 0, 0, | |||
NULL_PNTRSTRING}; | |||
|
|||
/* Verify proof of one statement in source file. Uses wrkProof structure. | |||
Assumes that parseProof() has just been called for this statement. */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a typical pre-condition, so mark it with /PRE
src/mmveri.c
Outdated
@@ -18,11 +18,16 @@ struct getStep_struct getStep = {0, 0, 0, 0, 0, | |||
NULL_PNTRSTRING}; | |||
|
|||
/* Verify proof of one statement in source file. Uses wrkProof structure. | |||
Assumes that parseProof() has just been called for this statement. */ | |||
Assumes that parseProof() has just been called for this statement. | |||
If verifyFlag includes VERIFY_DVS, check for unused $d conditions on nondummy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This describes the second parameter, so start the line with /PARAM verifyFlag
src/mmveri.c
Outdated
@@ -103,6 +136,14 @@ char verifyProof(long statemNum) | |||
g_WrkProof.mathStringPtrs[step] = | |||
g_Statement[stmt].mathString; | |||
|
|||
if (verifyFlag & VERIFY_ESSENTIAL) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see where the usage of a $f is registered. If I call verifyProof with VERIFY_FLOATING, how is the reference of a floating hypothesis recorded in essVar?
src/mmveri.c
Outdated
/* Returns 0 if proof is OK; 1 if proof is incomplete (has '?' tokens); | ||
returns 2 if error found; returns 3 if not severe error | ||
found; returns 4 if not a $p statement */ | ||
char verifyProof(long statemNum) | ||
char verifyProof(long statemNum, flag verifyFlag) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function is way to long, and the loop traversing the proof steps has several 'continue' shortcuts built in. Without interspersed post-condition assertions I find it hard to check for correctness. Consider extracting blocks of code to separate helper routines with a telling name.
printLongLine(g_Statement[g_Statement[statemNum].reqHypList[i]].labelName, | ||
"", " "); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indentation is out of order?
src/mmcmds.c
Outdated
if (verifyFlag) { | ||
if (verifyProof(i) >= 2) errorFound = 1; | ||
if (verifyFlag & VERIFY_FULL) { | ||
if (verifyProof(i, verifyFlag) >= 2) errorFound = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems that VERIFY_FULL is ignored in verifyProof. Why is this flag submitted to this function?
src/mmcmds.c
Outdated
/* If verify = 0, parse the proofs only for gross error checking. | ||
If verify = 1, do the full verification. */ | ||
/* If verifyFlag = VERIFY_NONE, parse the proofs only for gross error checking. | ||
If verifyFlag includes VERIFY_FULL, do the full verification. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this flag only controls a progress bar and a couple of result print outs. Please be more specific.
I haven't finished doing everything you requested yet. In particular, I have not done any refactoring of verifyProof. I'm pushing an update anyway so you can see my progress. I am traveling starting tomorrow, so won't get back to this probably until July 11 at the earliest. Note that I removed checking for unused $f statements, since that doesn't seem to make any sense. |
Hmm. trujust is an example with local |
How does the distinct variable part compare to https://github.com/metamath/set.mm/blob/develop/scripts/check-dvs.py ? |
I apologize for leaving this hanging. @jkingdon the script in question checks for variables that aren't mentioned at all in the theorem or proof. This PR, on the other hand, shows you dvs that the verifier never accessed while checking the proof, which implies that the verifier would still succeed if the dvs weren't present at all. @wlammen , that would be useful, but the code in the original version of this PR checked for unused $f statements globally, which produced voluminous, and not at all useful, output. I'll have to think about how to constrain it to local $f statements. |
src/metamath.c
Outdated
} else { | ||
verifyProofs(g_fullArg[2],1); // Parse and verify | ||
} | ||
i = (switchPos("SYNTAX_ONLY") != 0) ? 0 : 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here and below, isn't our convention to write conditions as (switchPos("SYNTAX_ONLY"))
instead of (switchPos("SYNTAX_ONLY") != 0)
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right. I have changed the code accordingly.
I heavily used these functionalities to optimize https://groups.google.com/g/metamath/c/OB2_9sYgDfA and they are quite amazing. While the Python script found this PR as result (which is about 50 or so eliminable dv conditions), the /DISJOINT_VARS option currently finds 14,406 dv conditions that can be eliminated in set.mm. Only problem is that the /FLOATING and the /HYPOTHESES options currently do not work @jamesjer and they don't even appear in the list of valid options. But once everything is settled I'm strongly in favour of merging this PR. EDIT: I just read above that checking for unused $f statements was intentionally removed, therefore it's ok with me. |
/DISJOINT_VARS checks for unused non-dummy dvs /ALL_DISJOINT_VARS check for all unused dvs /ESSENTIAL checks for unused $e statements
@GinoGiotto Thank you for the comments. I will note that we should be cautious about removing anything based on these checks. Some "unused" dvs and essential hypotheses are there on purpose, even if the verifier doesn't use them when verifying. For example, ax6v has an unused dv that is the entire point of ax6v, and a1ii has an unused $e on purpose. |
I know :-) Indeed I was thinking that an exception list might be a good idea. For dv conditions a good rule of thumb could be to exclude checking those theorems with a label |
Perhaps we could have a list of such theorems in the set.mm repo. This way, if someone wants to add extra DVs or hypotheses in a mathbox theorem for some reason, metamath.exe wouldn't have to change, and people wouldn't need to reinstall it. I am very interested in this pr; both the DV and hypothesis checker seems very useful |
I think this would be required if we want to move forward with this (perhaps in $j directives or some similar mechanism). I can think of a few examples in iset.mm which aren't in the list posted in this issue and I expect we'll add more in the future. |
I'm probably often using too many DV conditions, this seems kind of wrong, but since this is completely unchecked, there is no easy way to fix this, and this PR would help. |
Using $j directives is a good idea, they wouldn't be invasive since there probably aren't many theorems to be excluded. If there aren't other known issues, I encourage moving forward with this PR. In set.mm there is also axsep to be added in the list. |
The $j-mechanism sounds good. So, there are two cases, like
? For the second item, since DV conditions do not have labels, there is no canonical way to refer to them in a $j-statement. For instance, if I write Also, in both cases, some hypotheses or DV conditions may be intentional for some theorems and not others, a situation which may arise in the case of long scopes. This is one more reason to avoid long scopes, which are error-prone. |
I think |
Regarding the discussion at metamath/set.mm#2648, here is one way metamath-exe could be enhanced to show some unused elements of metamath databases. This PR adds the following qualifiers to
VERIFY PROOF
: