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

Pass optional message to assert #116

Closed
milancurcic opened this issue Jan 20, 2020 · 16 comments
Closed

Pass optional message to assert #116

milancurcic opened this issue Jan 20, 2020 · 16 comments
Labels
idea Proposition of an idea and opening an issue to discuss it topic: utilities containers, strings, files, OS/environment integration, unit testing, assertions, logging, ...

Comments

@milancurcic
Copy link
Member

Problem

Current implementation of assert in stdlib_experimental_error always prints the same message on failure:

subroutine assert(condition, code)
  logical, intent(in) :: condition
  integer, intent(in), optional :: code
  if (.not. condition) call error_stop("Assert failed.", code)
end subroutine

As is, if asserts fail, they tell you that they failed, but nothing more.

It's quite useful to be able to pass a custom message to assert. For example:

call assert(x < 3, "Error: x must be < 3")

would tell you why the assertion failed. This approach is especially useful during development, but could also be useful in production, for example to validate user inputs.

This is equivalent to Python's:

assert x < 3, "Error: x must be < 3"

Solution

This implementation would allow the user to optionally pass a custom message:

use stdlib_experimental_optval, only: optval

subroutine assert(condition, msg, code)
  logical, intent(in) :: condition
  character(*), intent(in), optional :: msg
  integer, intent(in), optional :: code
  if (.not. condition) call error_stop(optval(msg, "Assert failed."), code)
end subroutine

The solution is straightforward and backward compatible, assuming any existing code passed the optional code as a keyword argument.

I'd like to get support here before writing up a spec and a PR (both will be quite straightforward).

@certik @nncarlson @jvdp1 @ivan-pi @rweed What do you think? Anything I missed?

This issue is related to #72.

@milancurcic milancurcic added idea Proposition of an idea and opening an issue to discuss it topic: utilities containers, strings, files, OS/environment integration, unit testing, assertions, logging, ... labels Jan 20, 2020
@jvdp1
Copy link
Member

jvdp1 commented Jan 20, 2020

I think it is a good idea. I was developping a bit and got struggles to find which assert retunrs .false..
Maybe msg should be trimmed in optval. How is it assumed that the user would take care of it?

  if (.not. condition) call error_stop(optval(trim(msg), "Assert failed."), code)

Currently, there is only test_always_skip.f90 that is using code through call assert(.false., 77).

@milancurcic
Copy link
Member Author

Hmm, why would you want to trim it? msg is a character(*), so it will be always be of length of the input string. If the user (for whatever strange reason) passes a msg padded with spaces, I don't see why stdlib should trim that.

Currently, there is only test_always_skip.f90 that is using code through call assert(.false., 77).

Good to know, thanks for scoping it out. If we implement this, we should fix this test to state code=77 as keyword argument. This may be a good item for #3: pass optional arguments as keyword arguments to prevent breakage when the API changes.

@ivan-pi
Copy link
Member

ivan-pi commented Jan 20, 2020

I looked through the functions and see no reason for trimming either, both optval and error_stop accept character(*), intent(in) dummy variables.

This may be a good item for #3: pass optional arguments as keyword arguments to prevent breakage when the API changes.

We should at least exempt optval from such a rule. But otherwise I agree it is a good practice to follow.

@nncarlson
Copy link
Member

I'm unclear on the intended scope of this assertion code. Perhaps someone could orient me. Is this something meant for the testing framework only as @certik may have suggested or is meant for general use? Is there any relationship to a standards proposal for an ASSERT statement? Sorry, I haven't been following this thread as closely as I would have liked. I do use assertions in my own code, and have formed some very definite ideas about how they should work.

@jvdp1
Copy link
Member

jvdp1 commented Jan 20, 2020

Hmm, why would you want to trim it? msg is a character(*), so it will be always be of length of the input string. If the user (for whatever strange reason) passes a msg padded with spaces, I don't see why stdlib should trim that.

I was thinking to the scenario you described. But I agree with your point of view.

This may be a good item for #3: pass optional arguments as keyword arguments to prevent breakage when the API changes.

Indeed, a good practice to follow (except for optval IMO).

@milancurcic
Copy link
Member Author

@nncarlson I don't think there's a defined scope for stdlib's assert yet, or at least we haven't discussed it yet. I think we should. I also don't think we discussed any further the testing framework mentioned in #72.

Here's my wishlist for assert:

  • It's a subroutine that takes an input logical scalar argument and stops the program if its value is .false.;
  • Like Python, it can optionally receive an input character string to emit a custom error message (this proposal);
  • Optionally, the user can choose to emit an error message but not stop the program. This could be done with an optional logical warn argument;

And that's it. I used asserts similar to this in both datetime-fortran and functional-fortran. I'd use them in other projects.

Other optional capabilities that I don't desire but wouldn't oppose are:

  • Optionally stop the program with an exit code
  • Emit source file name and line number (a-la __FILE__ and __LINE__ cpp macros);

I do use assertions in my own code, and have formed some very definite ideas about how they should work.

Great! Can you share them?

@nncarlson
Copy link
Member

I think what is being imagined in the discussion here is more what I think of as a general "error handling" capability that is similar in spirit to Fortran namelists, which provide an input parsing capability that is good enough for a great many use cases. In the same vein, the error handling functions being discussed here provide value and are mostly good enough. I do believe there is a place in stdlib for such a thing.

I would not call these assertions, though. What I'd like to see as part of standard Fortran is an assertion capability with a much more limited scope similar to what is in C. Assertions are for catching programmer errors -- things like mismatched dummy array sizes, pre-conditions, post-conditions, etc. -- and not for catching errors that are ultimately due to program input. Those require proper error handling like what might be provided by the above. Assertions should be:

  • trivial to use, just ASSERT(scalar-logical-expression); no need to use a module.
  • if tripped, print out file, line #, and ideally the condition (as a string), and abort execution.
  • if not compiled in debug mode, completely disappear (absolutely no overhead). This probably means ASSERT would be a statement and not a procedure call.

In the meantime, one can come close to this using the preprocessor and an ASSERT macro, which is what I currently do. This does require putting a #include statement at the top of files to include a header file that defines the macro (this is a nuisance). Something like

#ifdef NDEBUG
# define ASSERT(x) !assert( x )
#else
# define ASSERT(x) if(.not.(x)) call f90_assert(__FILE__,__LINE__)
#endif

f90_assert would necessarily be an external subroutine.

I wouldn't object to this form of an assertion taking optional arguments like an error message, but I think that would require an explicit interface (unless ASSERT were part of the language), and seems like scope creep toward a more general error handling system.

Maybe we could split things into a low-level assertion similar to what I describe and a separate higher-level error handling capability?

@certik
Copy link
Member

certik commented Jan 21, 2020

@nncarlson I think what you are proposing is a solution that works today, until j3-fortran/fortran_proposals#70 gets implemented in the language itself.

@milancurcic
Copy link
Member Author

I don't object to a minimal C-like assert like this. Should the current implementation of assert then be expanded to print file name and line?

In the same vein, the error handling functions being discussed here provide value and are mostly good enough. I do believe there is a place in stdlib for such a thing.

Yes, I'd personally find such subroutine more useful than a C-like assert. Basically what I'm looking for is the current implementation of assert in stdlib_experimental_error + emitting a custom message + optionally warn-only. How should this be called? test_condition?

@certik where are you at on this?

@jvdp1
Copy link
Member

jvdp1 commented Jan 22, 2020

I don't object to a minimal C-like assert like this. Should the current implementation of assert then be expanded to print file name and line?

That would be something nice to have at least file name and line in the output of assert. When implementing #119 , I added severeal assert in once in the test file, and tool me some times to find which one returns .false..

Yes, I'd personally find such subroutine more useful than a C-like assert. Basically what I'm looking for is the current implementation of assert in stdlib_experimental_error + emitting a custom message + optionally warn-only. How should this be called? test_condition?

Is it (in some way) related to #95 ? Or could both idea be merged?

@milancurcic
Copy link
Member Author

Is it (in some way) related to #95 ? Or could both idea be merged?

I think the ideas are related and could have similar use cases. However I am personally not in favor of #95 (for me it's overkill). What I described in the previous message is exactly what I wish for.

I added severeal assert in once in the test file, and tool me some times to find which one returns .false..

This problem could be easily fixed with this proposal :). It's also how I write tests, but I'm not married to the idea of it being called assert.

@nncarlson
Copy link
Member

@nncarlson I think what you are proposing is a solution that works today, until j3-fortran/fortran_proposals#70 gets implemented in the language itself.

Exactly. In fact #72 started as just that, but quickly got diverted into a discussion about more elaborate error handling facilities. Again, I'm in no way opposed to such things, but they solve a different problem. Traditional assertions (and I'd rather the more elaborate error handling not co-opt that terminology) are strictly about catching unexpected programmer errors and should never be tripped (implemented by the compiler they can also declare conditions used by the optimizer). Other "expected" errors need a different solution. Perhaps one way to help distinguish the two types is to ask whether it is appropriate for the error check to be omitted in a non-debug build.

@milancurcic
Copy link
Member Author

milancurcic commented Jan 22, 2020

Perhaps one way to help distinguish the two types is to ask whether it is appropriate for the error check to be omitted in a non-debug build.

Yes, I think this is the key distinction. I'd like to be able to test conditions with meaningful messages in both debug and release builds.

Okay, it seems from this thread that there is now a more-or-less clearly defined scope for assert based on input from @nncarlson and @certik. This proposal shouldn't touch assert then.

I will close this issue tomorrow and open a new proposal for test_condition to elicit feedback.

@certik
Copy link
Member

certik commented Jan 22, 2020

Yes, indeed, I currently use the assert function for both tests and inside code. Rather, it should be split, and there should be a version used for tests, which will be executed in Release mode also. And then there should be a version to be used inside code in Debug mode only (no overhead in Release mode), implemented using a macro.

@urbanjost
Copy link

I have my own preprocessor, which has a debug mode than only includes $ASSERT lines and blocks of lines starting with "DEBUGVERSION: block" to "endblock DEBUGVERSION" only if debug mode is enabled and activated (that is, they are either both in the code or excluded from the code). Code that tests values is just part of the normal code (no way to turn if off or exclude it). So I use something similar to what you are describing a lot,. The discussion got be looking because the (very simplistic) macro $ASSERT allows up to nine values as a message. I found that over eighty percent of the $ASSERT lines
added a message (file name and line number gets added automatically) so I would give a thumbs up on a standard ASSERT directive, but vote for the optional message being available. But that seems like it should be a request for enhancement in the standard more than somthing in stdlib? As a footnote, the nine message values are class(*), which makes it really easy to build a useful message
(like the tostring example on the Fortran WIki).

I find that method extremely useful for building relatively arbitrary messages, albeit it requires unlimited polymorphic variables, which would probably exclude a lot of compilers from using a similar procedure. But if an ASSERT directive were to become part of the Fortran standard I would really like it to have a message that was similar, and think it would be useful if the message actually showed the logical test (which is something I keep meaning to add to my own $ASSERT directive and never seen to get around to :>).

@milancurcic
Copy link
Member Author

Closing in favor of #121.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
idea Proposition of an idea and opening an issue to discuss it topic: utilities containers, strings, files, OS/environment integration, unit testing, assertions, logging, ...
Projects
None yet
Development

No branches or pull requests

6 participants