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

Allow disable-non-linear-arithmetic per module #4773

Merged

Conversation

keyboardDrummer
Copy link
Member

@keyboardDrummer keyboardDrummer commented Nov 14, 2023

Description

Allow {:disable_nonlinear_arithmetic} on any module, with the same effect as --disable-nonlinear-arithmetic but scoped only to the definitions in that module.

How has this been tested?

Source/IntegrationTests/TestFiles/LitTests/LitTest/verification/nonLinearArithmetic.dfy

By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.

@atomb atomb marked this pull request as ready for review November 17, 2023 21:45
@keyboardDrummer keyboardDrummer enabled auto-merge (squash) November 20, 2023 11:48
Copy link
Member

@MikaelMayer MikaelMayer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! I'm happy that this CLI option can become an attribute

@@ -0,0 +1,104 @@
// RUN: ! %verify "%s" --disable-nonlinear-arithmetic --resource-limit 1000 > "%t"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would put two RUN here, one with --disable-non-linear-arithmetic and one without. That way, we can verify that the output of Power1 no longer depends on this flag.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

@@ -2274,6 +2274,10 @@ and what information it produces about the verification process.
operators. (This option will perhaps be replaced by `-arith` in the
future. For now, it takes precedence over `-arith`.)

The behavior of `disable-nonlinear-arithmetic` can be turned on and off on a per-module basis
by placing the attribute `{:disable-nonlinear-arithmetic}` after the module keyword.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
by placing the attribute `{:disable-nonlinear-arithmetic}` after the module keyword.
by placing the attribute [`{:disable-nonlinear-arithmetic}`](#sec-disable-nonlinear-arithmetic) after the module keyword.
  • please add an entry into the file Attributes.md

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added it, although I do not think it makes sense to group documentation based on something being an attribute not. When I'm dealing with a particular topic, I would like to be told what language features, including attributes, I can use regarding that topic. When will I ever think, "I'd like to use attributes", and then go look for documentation specific to them?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand that when one is presented with a concept, they will want to know all the language features that allow them to use that concept. I totally agree with that, I want this too. This is the explanation part of the documentation.
To give sense to group documentation based on something being an attribute or not, let's consider the following:
We all work and read in different ways, and this is a reason why we add links to the documentation to group concepts in different ways. Connectivity enables discoverability.
With your edit, if I browse the able of contents, I will now see this:

image

Users looking for attributes that could reduce the brittleness of proofs might not think about non-linearity, but might be looking for "reveal" and find {:autoRevealDependencies} or {:fuel}., and see that there is something about nonlinear arithmetic a few attributes above that catches their attention

Or users thinking that hey I have so many lemmas that are hard to prove, could want to use extern to prove them and import their proofs. They look for {:extern} and see the {:disable_nonlinear_arithmetic} attribute just before that catches their attention and actually could just be something they were needing.

I hope it helps!

Copy link
Member Author

@keyboardDrummer keyboardDrummer Nov 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Users looking for attributes that could reduce the brittleness of proofs

Why would a user want to use attributes specifically? If they run into issues with brittleness, wouldn't they look for a section that's about just that? Like the "Debugging brittle verification" one. And then I would hope everything they need to know, whether it concerns attributes or not, is in there.

For me grouping by attributes is like grouping by all types of syntax that start with (. It's a grouping based on appearance instead of meaning, which doesn't seem useful to me.

I think the reason someone might have started a section with just attributes was because it allows you to add something to the documentation without having to think about where to place it: this section may contain anything as long as it is an attribute. I think it would be better for our users if we remove that section entirely, both so that developers do not get the option to only add something to that list but nowhere else in the manual, and so that users don't have to unnecessarily follow links in the manual. I'll put an item on my TODO list for that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why would a user want to use attributes specifically?

They might be reading a code base and looking for the meaning of some attributes they find. In the future, we could provide documentation over attributes based on this section to explain what these attributes mean. In another future, when we will migrate attribute to proper typed Dafny datatypes, these paragraphs could serve as docstring.

I think it would be better for our users if we remove that section entirely, both so that developers do not get the option to only add something to that list but nowhere else in the manual, and so that users don't have to unnecessarily follow links in the manual.

What you are writing makes no sense to me. As a Dafny developer, I want to get the option to add something to that list before I find a suitable place to integrate it in the manual (e.g. later as part of a guide about how to reduce brittleness). Second, following links is the standard in the web documentation so I see no reason why to remove connected components.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the sweet spot would be having a section that lists all the attributes for various declarations, but only as an index into sections that talk about the actual relevant features.

Definitely another +1 on #3149 providing an even better solution in the future though - I'd still much rather Go to Definition on an attribute reference in code to learn about it than to have to go to the reference manual at all!

@MikaelMayer MikaelMayer disabled auto-merge November 20, 2023 11:59
@keyboardDrummer keyboardDrummer enabled auto-merge (squash) November 20, 2023 12:27
MikaelMayer
MikaelMayer previously approved these changes Nov 20, 2023
@keyboardDrummer
Copy link
Member Author

Need another Boogie update: boogie-org/boogie#811

@keyboardDrummer keyboardDrummer merged commit b1cda95 into dafny-lang:master Nov 24, 2023
20 checks passed
@keyboardDrummer keyboardDrummer deleted the nonLinArithPerModule branch November 25, 2023 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants