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

Do something about @assert #51729

Open
LilithHafner opened this issue Oct 17, 2023 · 14 comments
Open

Do something about @assert #51729

LilithHafner opened this issue Oct 17, 2023 · 14 comments
Labels
feature Indicates new feature / enhancement requests

Comments

@LilithHafner
Copy link
Member

@assert seems like a pretty useless macro right now.

Here are some plausible uses for @assert

  1. Input validation in a public-facing function
  2. Check invariants in the middle of an algorithm to give errors if the programmer made a mistake
    a. When small constant factors do matter
    b. When small constant factors do not matter
  3. Give the compiler a hint that a statement is true to improve performance.

Right now @assert is only useful in 2.b.

@elextr
Copy link

elextr commented Oct 17, 2023

@assert is a debugging tool, not a convenience for normal coding.

Because it can be removed by "various" optimisation levels (would be good to document) it is possible to have it present for debugging runs, but removed from production runs without editing the source. So the test can be expensive as it is only present in debugging runs.

But that means it is intentionally not suitable for any use-case where a permanent test is required, so it cannot be relied on to provide any of the cases in the OP.

An equivalent non-removable convenience macro (@ensure maybe) would probably be useful though and could provide all of the use-cases in the OP.

Edit: I recently came across the same problem recently in a C++ case where code was using assert to check and then production builds failed to validate inputs.

@brenhinkeller brenhinkeller added the feature Indicates new feature / enhancement requests label Oct 17, 2023
@LilithHafner
Copy link
Member Author

So the test can be expensive as it is only present in debugging runs.

Nay, @assert is not removed at any optimization level that I'm aware of.

@elextr
Copy link

elextr commented Oct 17, 2023

Nay, @Assert is not removed at any optimization level that I'm aware of.

Then the docs for assert are wrong.

It is actually probably good that it does not remove by optimisation, makes debugging optimised builds difficult, might be good to have an NDEBUG like C/C++ to enforce removal/inclusion separate from optimisation.

@KristofferC
Copy link
Member

The docs say it may be removed, that it doesn't yet is something that may be changed, see eg #37874

@KristofferC
Copy link
Member

And as been said, this is a debugging tool to check that internal invariant are upheld so it isn't suitable for 1 or 3. 2a would be solved with something like the PR I linked above.

I think this issue can be closed since there doesn't really seem to be anything actionable to do here.

@LilithHafner
Copy link
Member Author

there doesn't really seem to be anything actionable to do here.

I want to take exactly one of the following actions:
X) Disable @assert in non-debug code; or
Y) Give up and declare that we will never disable @assert.

I don't like the status quo of threatening to disable @assert but never following through.

@ParadaCarleton
Copy link
Contributor

It leads to bad programming practices as well, where people refuse to add @assert on anything for performance reasons, but also avoid it for necessary checks because it "might be disabled."

@KristofferC
Copy link
Member

but also avoid it for necessary checks because it "might be disabled."

Well yes, if the checks are necessary you should be using something else than an assert. Doing something like

@assert password_is_ok(pwd)

wouldn't be a good idea.

@elextr
Copy link

elextr commented Oct 19, 2023

you should be using something else than an assert.

Which is why I suggested adding @ensure (or other name) that is not removable, giving the ease of use and fail-fast of @assert without the risk of removal.

There should also be the ability to manually enable/disable @assert (like C/C++ NDEBUG) to support use-cases like:

for hot_loop:
    @assert expensive_test
    ...
end

so the user can know its tested in debugging even at maximum optimisation, and know it won't slow production even on low optimisation.

@adienes
Copy link
Contributor

adienes commented Oct 28, 2023

+1 that it would actually be a super nice feature if I could put @assert invariants into functions with impunity, even if they are in tight loops, and know that they will still compile fast when disabled

@ericphanson
Copy link
Contributor

I think there should be 3 separate functions for these three separate needs (eg @check, @assert, and @assume), and they should all be in Base so @assert isn't privileged which tempts folks to abuse it.

I gave adding @check a try but my PR stalled as it was adding a new thing (which is hard) and the thing wasn't as good as ArgCheck (which has more code and thus harder to get added), aka a variant of logrange problem.

@jariji
Copy link
Contributor

jariji commented Jan 25, 2024

@ericphanson What does @assert do in your 3-way scheme?

@ericphanson
Copy link
Contributor

Item (2) from the OP; a check that is only guaranteed to run in a debug mode or such. (basically the current one but we should also let the compiler skip it if advantageous and are not in a debug mode)

@nsajko
Copy link
Contributor

nsajko commented Jul 21, 2024

xref #10614

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Indicates new feature / enhancement requests
Projects
None yet
Development

No branches or pull requests

9 participants