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

Add assert macro #5065

Closed
wants to merge 2 commits into from
Closed

Add assert macro #5065

wants to merge 2 commits into from

Conversation

Sija
Copy link
Contributor

@Sija Sija commented Sep 30, 2017

Extracted from #4263 by @will.

/cc @oprypin

@ysbaddaden
Copy link
Contributor

Macros no longer take precedence over methods named the same?

@RX14
Copy link
Contributor

RX14 commented Oct 1, 2017

This can be specced, so it should have specs.

@oprypin
Copy link
Member

oprypin commented Oct 1, 2017

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
Copy link
Contributor Author

Sija commented Oct 1, 2017

@oprypin Could this be a solution?

->{ ... }.call

@RX14
Copy link
Contributor

RX14 commented Oct 1, 2017

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

@oprypin
Copy link
Member

oprypin commented Oct 1, 2017

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

@RX14
Copy link
Contributor

RX14 commented Oct 1, 2017

@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
Copy link
Contributor Author

Sija commented Oct 1, 2017

@oprypin @RX14 updated with working solution.

@asterite
Copy link
Member

asterite commented Oct 3, 2017

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

@oprypin
Copy link
Member

oprypin commented Oct 3, 2017

Yeah now paste that link on the unreachable proposal

@RX14
Copy link
Contributor

RX14 commented Oct 3, 2017

@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
Copy link
Contributor Author

Sija commented Oct 5, 2017

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

@Sija
Copy link
Contributor Author

Sija commented Oct 5, 2017

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

Copy link
Contributor

@ysbaddaden ysbaddaden left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

Sija commented Dec 16, 2017

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

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

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

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

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
Copy link
Contributor

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

@Sija
Copy link
Contributor Author

Sija commented Dec 17, 2017

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

@Sija
Copy link
Contributor Author

Sija commented Dec 21, 2017

@ysbaddaden hello?

@ysbaddaden
Copy link
Contributor

Well removed or replace or something, I guess.

@Sija
Copy link
Contributor Author

Sija commented Dec 21, 2017

@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
Copy link
Contributor

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
Copy link
Contributor

RX14 commented Dec 21, 2017

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

@pkoppstein
Copy link

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
Copy link
Contributor

@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
Copy link

@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
Copy link
Contributor

ysbaddaden commented Jan 7, 2018

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
Copy link
Contributor

RX14 commented Jan 7, 2018

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.

@RX14 RX14 closed this Jan 7, 2018
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.

None yet

7 participants