Skip to content
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

Refactor witness output interpretation #982

Merged
merged 3 commits into from
Dec 11, 2023
Merged

Refactor witness output interpretation #982

merged 3 commits into from
Dec 11, 2023

Conversation

marian-lingsch
Copy link
Contributor

In order to make it easier to understand the output of the witness linter we sepparate the exception case from the case where there was no output

In order to make it easier to understand the output of the witness linter we sepparate the exception case from the case where there was no output
@PhilippWendler
Copy link
Member

Does "witnesslint finished" not in run.output[-1] really imply an EXCEPTION result?

@marian-lingsch
Copy link
Contributor Author

If witnesslint exited successfully, the last line will always contain "witnesslint finished".
If this line is not present something went wrong internally, which is usually due to some exception, therefore we return EXCEPTION in this case.
The same could be reasoned for an empty output. Is it preferable to return EXCEPTION (no output) in this case or Error (no output) in this case?

@PhilippWendler
Copy link
Member

It is a decision of the tool maintainer what to do here, but I personally would find the "usually" too weak here. Nothing worse during debugging than some output that leads you in the wrong direction. In CPAchecker's tool-info module, for example, we return EXCEPTION if there is some specific output that indicates that there was an exception. So if I was responsible for the linter's tool-info module, I would return ERROR (no output) as it is suggested, maybe EXCEPTION for exit code 7 (if this is what the tool produces), potentially EXCEPTION if there is something that looks like an exception in the output, and as a fallback ERROR if none of these and the other existing cases match.

Btw.: I just noticed the UNKNOWN return value. UNKNOWN is for cases where the tool voluntarily gives up and does not attempt to provide an answer (like an incomplete verifier). It is not for cases where the tool-info module fails to find out a proper return value. In the code here it looks like the latter case, so I guess it should be changed to return ERROR.

Marian Lingsch-Rosenfeld added 2 commits December 11, 2023 09:27
Unknown should only be returned if the tool voluntarily gave up, which is not the case here
…ption

We should only return exceptions when we are sure that an exception has occured. If it is only the case that the linter did not finish successfully we should return an error
@marian-lingsch
Copy link
Contributor Author

Thank you very much for your comments. I tried to address both concerns with the last two commits by returning an Error per default and splitting the error that witnesslint did not finish from the case where we know there has been an exception.
Feedback is always very welcome!

@PhilippWendler PhilippWendler merged commit 19a85ac into main Dec 11, 2023
6 checks passed
@PhilippWendler PhilippWendler deleted the witness-linter branch December 11, 2023 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants