Add assert macro

[Sija]

Extracted from #4263 by @will.

/cc @oprypin

[ysbaddaden]

Macros no longer take precedence over methods named the same?

[RX14]

This can be specced, so it should have specs.

[oprypin]

One problem with this implementation is that it causes different type deductions in debug and release mode.

a = 5 if true
assert a.is_a? Int32
p a + 7

Works in debug, gets an error in release mode undefined method '+' for Nil

It would be really nice to keep the type limitation but drop the runtime check that it is actually that type. Though then it may be deemed too dangerous even for release mode.

[Sija]

@oprypin Could this be a solution?

->{ ... }.call

[RX14]

@Sija i think && true or maybe && CONST_TRUE where CONST_TRUE is defined to be always true would trick the type inference.

[oprypin]

@RX14, && can't do the trick no matter what. Inference just ignores the other operand.

[RX14]

@oprypin no.

foo = "foo" || 1
if foo.is_a?(String) && false
  raise "bar"
end

# Foo will always be `String | Int32` here

You're correct in that && true won't affect the type inference inside the if, because it's guaranteed that all conditions are met. However, it does affect type inference after the if, because you can no longer guarantee that the branch will be taken.

Given we don't use {{exp}} inside the if, this solves our problem.

[Sija]

@oprypin @RX14 updated with working solution.

[asterite]

https://golang.org/doc/faq#assertions

[oprypin]

Yeah now paste that link on the unreachable proposal

[RX14]

@oprypin but unreachable is about telling the compiler that a certain case is logically impossible to happen. This is a case which is simply impossible to ever achieve. If an unreachable method is ever reached, then there's a bug in the current method.

[Sija]

@RX14 I couldn't find macros spec, so I didn't add any. Did I miss 'em/should I create new spec?

[Sija]

@asterite FYI, Swift has both, assert along with precondition.

[ysbaddaden]

I won't object if it eventually gets merged, but my vote goes to reject this.

As Asterite pointed out, proper arguments checks and raising ArgumentError with an explanatory message should be implemented instead.

[Sija]

Just found this macro buried inside of Float::Printer::* impl.
Looks like assert in disguise to me...

crystal/src/float/printer/cached_powers.cr

Lines 167 to 173 in a1e90f0

	private macro _invariant(exp, file = __FILE__, line = __LINE__)
	{% if !flag?(:release) %}
	unless {{exp}}
	raise "Assertion Failed #{{{file}}}:#{{{line}}}"
	end
	{% end %}
	end

crystal/src/float/printer/diy_fp.cr

Lines 143 to 149 in a1e90f0

	private macro _invariant(exp, file = __FILE__, line = __LINE__)
	{% if !flag?(:release) %}
	unless {{exp}}
	raise "Assertion Failed #{{{file}}}:#{{{line}}}"
	end
	{% end %}
	end

crystal/src/float/printer/grisu3.cr

Lines 356 to 362 in a1e90f0

	private macro _invariant(exp, file = __FILE__, line = __LINE__)
	{% if !flag?(:release) %}
	unless {{exp}}
	raise "Assertion Failed #{{{file}}}:#{{{line}}}"
	end
	{% end %}
	end

crystal/src/float/printer/ieee.cr

Lines 195 to 201 in a1e90f0

	private macro _invariant(exp, file = __FILE__, line = __LINE__)
	{% if !flag?(:release) %}
	unless {{exp}}
	raise "Assertion Failed #{{{file}}}:#{{{line}}}"
	end
	{% end %}
	end

I'd say it should be either removed or replaced with assert from this PR.

[ysbaddaden]

Because Grisu3 it was ported from C as far as I recall.

[Sija]

@ysbaddaden indeed, yet what should we do about camouflaged assert macro in there?

[Sija]

@ysbaddaden hello?

[ysbaddaden]

Well removed or replace or something, I guess.

[Sija]

@ysbaddaden Could you be more specific about the next actions? replace or sth is a poor answer and this PR is exactly one of the proposed solutions (replace with assert macro), should it be merged then?

[ysbaddaden]

Sorry, I have no better response. I consider assert(condition) to be a bad practice: no error message, no validation in release mode, impossible to activate in release or deactivate in non release. They should be proper error checking.

Hence my opinion: drop or replace disguised assertions —and closed this pull request, too.

[RX14]

I agree with @ysbaddaden, let's just remove _assert and all it's calls.

[pkoppstein]

I am new to Crystal but am glad to see that serious consideration
is being given to adding core support for assertions. In the
following, I would like to propose a slightly more flexible
approach than is offered by C-style assertion facilities.

INTRODUCTION

This reference implementation of a simple assertion facility for
Crystal provides the functionality usually expected of such a
facility but adds run-time flexibility. Specifically, unless the
"--release" flag has been specified, assertion checking can be
turned on and off at run-time, and similarly a run-time flag can be
toggled on and off to determine whether an assertion violation
should trigger an exception or an error message.

Rationale

Assertions have an important role to play in application code (see
e.g. [*1]) but rigid adherence to the view that assertion checking
must either be disabled or result in run-time errors on violations
often results in their under-utilization. Indeed, a case can be
made for retaining assertion-checking even in production code, on
the understanding that the checking of assertions and the reporting
of violations will both be orthogonal to application functionality.

Example

AssertionError.off
assert(1 == 3, "expected violation")
Assertion.off
assert(1 == 2, "unexpected message")
Assertion.on
AssertionError.on
assert(3 == 2)

The first assert in this example results in a message such as:

Assertion violation: /Users/xyzzy/app.cr:85: 1 => 1 == 3 <= 3: expected violation

ACKNOWLEDGEMENTS

This implementation is partly based on prior work on assertions in Crystal and on
https://blaxpirit.com/blog/17/detailed-assert-macro-for-crystal.html

Reference

[*1] https://www.cs.ox.ac.uk/files/6184/H2002%20-%20Assertion.pdf

REFERENCE IMPLEMENTATION

This implementation defines two classes (AssertionError and
Assertion) and a global method for making run-time assertions:
assert(cond)
assert(cond, msg)

The behavior of assertions is governed by the Crystal "release" flag
and by the boolean values: Assertion.status and AssertionException.status

These .status values can be turned on or off by calling #on or #off, e.g.
Assertion.off # turns assertion checking off

Here is a more detailed specification:

If the "release" flag has been specified, all assertions are identical to true;

• otherwise, if Assertion.status is false at the time an assertion is encountered,
  the assertion evaluates to true;
• otherwise, if the condition is satisfied, the assertion evaluates to true;
• otherwise, if AssertionException.status is true, an AssertionException is raised with a message;
• otherwise, a message is directed to STDERR.

The message emitted includes the FILE and LINE number of the assertion,
details about the condition, and msg if specified.

class AssertionError < Exception
  @@on = true
  def self.status
    @@on;
  end
  def self.on
    @@on = true;
  end
  def self.off
    @@on = false
  end
end

class Assertion
  @@on = true
  def self.status
    @@on;
  end
  def self.on
    @@on = true;
  end
  def self.off
    @@on = false
  end
end


macro assert(cond, msg = nil, file = __FILE__, line = __LINE__)
  {% if flag?(:release) %}
    true
  {% else %}
    if Assertion.status == false 
      true
    elsif {{ cond }}
      true
    else
      %error = "#{{{file}}}:#{{{line}}}"
        {% if cond.is_a? Call && %w(== != < > <= >=).any? {|s| s == cond.name.stringify} %}
          {% a = cond.receiver; b = cond.args[0] %}
          %error += ": #{{{ a.stringify }}} => #{{{ a }}} {{ cond.name }} #{{{ b }}} <= #{{{ b.stringify }}}"
        {% else %}
          %error += ": " + {{ cond.stringify }}
        {% end %}
      {% if msg %}
         %error += ": #{{{ msg }}}"
      {% end %}
      if AssertionError.status
        raise AssertionError.new(%error)
      else
        STDERR.puts "Assertion violation: #{%error}"
      end
      true
    end
  {% end %}
end

[lbguilherme]

@pkoppstein Turning on and off at runtime is awful because you pay the performance cost of having a check always. Please don't. Having a compile time flag should already cover enough usecase.

---

Having some kind of assertion is good not for validating arguments or checking runtime conditions, but for validating that your own algorithm/logic is correct. It checks the invariants you assumed to be true. If an assert is triggered, it is a bug in the code that contains the assert, not from the user, not from somewhere else.

That being said, I don't know how to prevent the feature being misused for checking arguments or other runtime conditions, like Go FAQ mentions.

What I once did that is not much different from this:

macro if_defined(x)
  {% if x.resolve? %}
    {{yield}}
  {% end %}
end

# inside some method:

        if_defined(Spec) do
          value.should be 0
        end

This runs the "assert" only during specs, that is the time to check if everything is behaving as they should. I'm not really sure this is good or bad practice though, but adds to the discussion.

[pkoppstein]

@lbguilherme wrote:

Turning on and off at runtime is awful because you pay the performance cost of having a check always.

I'm sorry you didn't take the time to understand the proposal more carefully. As it says quite prominently:

If the "release" flag has been specified, all assertions are identical to true

The reference implementation also makes it clear that when the --release flag is specified, the neither the assertion conditions nor the on/off flags are checked, because all that checking has been compiled away. All that's left of each assertion is "true".

Now it may be objected that when the --release flag is specified, everything to do with assertions should be completely compiled away. Practically speaking, a few additional trues, (most of which would surely be compiled away anyway) won't make a detectable difference in real-world programs, but I would be fine with removing them provided that the compiler is "assertion-aware" and can ensure that all assertions are "invisible" semantically, whether assertion checking is enabled or not. This could be quite a lot of work, which is why the proposal simply requires that assertions that don't raise an exception always have a fixed semantic value (i.e. true).

It checks the invariants ...

Here it seems you are attempting to impose one particular (and rather narrow) view (or definition) of assertions on everyone. As I sought to emphasize, the proposal seeks to increase flexibility, without taking away the possibility of easily using the assertions in more conventional ways. In fact, the defaults have all been set with such use in mind.

Although I believe using the word "assert" in connection with this proposal is appropriate, I am less concerned about the naming than the additional flexibility.

As I mentioned, I am fairly new to Crystal and am sure that the proposal can be improved. There are certainly several variants that would be worth discussing. However, my years of experience in a large software development organization have led me to conclude that the additional flexibility that I've mentioned makes a huge and positive difference in multiple ways, as suggested by the already-cited work by Tony Hoare.

[ysbaddaden]

I'm even stronger against assertions than before.

I did a bunch of C over the last month, and the language forces you to put assertions almost everywhere because we ASSUME A LOT, and we better have a solution to mark/verify these assumptions (which often catch bugs). In crystal, we bound check and prefer to use safe structures, we have a strong type system with generics (no need to cast pointers), even casting a union type to a more restricted type is safe. Hence we don't assume much, don't manipulate pointers much (use slices), and more importantly don't have to cast pointers to whatever every few lines.

We don't have to put assertions to verify claims in Crystal. We only need to properly raise an exception with an explanatory message to report invalid user provided values.

Hence: no assert in core. Please create a shard for the 0.1% of projects that may benefit from it (i.e. writing a garbage collector).

[RX14]

I think the core team is pretty much agreed on this. I'd like to see assert as something Spec provides (unfortunately not many others do :C) but absolutely not in core.