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

Use of assert/exceptions should follow the code style guide #99

Open
oberstet opened this issue Mar 14, 2012 · 10 comments
Open

Use of assert/exceptions should follow the code style guide #99

oberstet opened this issue Mar 14, 2012 · 10 comments

Comments

@oberstet
Copy link
Contributor

=> can be optimized away when doing compile for pyo's

@oberstet
Copy link
Contributor Author

asserts are only (currently) removed on CPy when doing -O.

On PyPy, asserts are always compiled and executed!

@oberstet
Copy link
Contributor Author

Should we use asserts? Should we extensively time-check arguments at run-time? Where and where not?

@oberstet oberstet added this to the 0.11.0 milestone Aug 25, 2015
@meejah
Copy link
Contributor

meejah commented Aug 25, 2015

My opinion is: we should use if/Exception or RuntimeError for things that will become user-visible (e.g. "invalid realm name" or "port must be an int") but use assert for things that are purely checking assumptions (e.g. any "can't arrive here" things etc). That is, things that really, really should never happen.

For one thing, most users when seeing a traceback from an assert are probably just going to file a bug...

Also, there should probably be at least one unit-test covering the assert-ed statement. Essentially, I view assert as a way to tell fellow programmers what should/shouldn't be true. However, you should still try your best to cover those cases with unit-tests ;)

@oberstet oberstet removed this from the 0.11.0 milestone Sep 6, 2015
@oberstet
Copy link
Contributor Author

pypy: http://paste.pound-python.org/show/yeHGJ8p41aVSEFx3gCKW/

>>> def f(i):
...   assert i, "a" % (i,)
... 
>>> import dis
>>> dis.dis(f)
  2           0 LOAD_FAST                0 (i)
              3 POP_JUMP_IF_TRUE        25
              6 LOAD_GLOBAL              0 (AssertionError)
              9 LOAD_CONST               1 ('a')
             12 LOAD_FAST                0 (i)
             15 BUILD_TUPLE              1
             18 BINARY_MODULO       
             19 CALL_FUNCTION            1
             22 RAISE_VARARGS            1
        >>   25 LOAD_CONST               0 (None)
             28 RETURN_VALUE        
>>> 

fijal (irc):

<oberstet> about assert() .. how does pypy treat that? is the assert code always run (never optimized away) on pypy?
<fijal> oberstet: assert usually helps you ;-)
<fijal> we compile special bytecode that has a check for assert
<oberstet> so you are +1 on using it extensively eg throughout a library to "harden" the public (and also internal) API?
<oberstet> eg. def foo(uri) .. and then assert(uri), "don't try to bug me! URI must be unicode"
<oberstet> also, is it safe/recommended to eg: assert(type(uri) == unicode), "URIs must be unicode - got {}".format(type(uri)) ?
<oberstet> IOW: is there any downside of using that last idiom on pypy extensively?
<fijal> assert is really if something: raise AssertionError(....)
<fijal> which means that it'll check for isinstance (a very cheap check)
<fijal> and then will not execute the formatting until it fails
<oberstet> perfect
<oberstet> so when the assert usually is OK, then the JIT will optimize for the non-taken branch in the "if" you mention?
<oberstet> sorry for asking again and again .. but I want to be "sure" before adding this in dozens and dozens of places
<fijal> well that's how ifs work in tracing JITs
<oberstet> great. I figured. but i wanted to be sure. thx!
<fijal> as in, the branch not taken will be not compiled at all unless it fails >200 times
<fijal> http://paste.pound-python.org/show/yeHGJ8p41aVSEFx3gCKW/
<fijal> oberstet: look at this, it really is like a normal if
<oberstet> hehe. very good. I need to learn more such disasm tricks for pypy. very useful.
<oberstet> thx!
* Ilirium has quit (Ping timeout: 268 seconds)
<fijal> that's a normal python trick too
* Ilirium (~Ilirium@212-192-120-168.IP.SUR.net.ru) has joined
<arigato> oberstet: note that you should ask on #python, because it's not as simple as that
<arigato> some would say "don't use asserts like that because they won't be run with 'python -O' or 'pypy -O' "
<oberstet> I know .. and I am fine with that. our main target is pypy.
<arigato> well, pypy or python, the answer is the same: -O disables asserts and has no other effect
<arigato> it's a bad idea to rely on the -O flag not being specified
<fijal> (which means they're not run)
<fijal> IMO it's equally bad to run your python with -O
<oberstet> huh? I thought pypy does _never_ do away with asserts - even _with_ -O?
<arigato> no, pypy follows cpython nowadays
<oberstet> which I thought is what I was asking ..
<oberstet> ok
<arigato> (it used to be the case that it works as you describe)
<oberstet> yeah, I read sth like this in the past ..
<oberstet> some distro apparently install py packages with -O

@oberstet
Copy link
Contributor Author

@meejah

That is, things that really, really should never happen.

Like a user calling session.call with a URI not of type Unicode? Because that is what the interface says?

Should we assume users can't read / are dumb? ;)

@oberstet
Copy link
Contributor Author

Say we would use assert only for this: if an assert fails, it means we have a bug within the library

So we wouldn't use assert for checking public API entry points into the library. My main concern.

How do we do that?

We can't use the new type annotations, as these are just for static code analysis. Right?

So we would use plain old:

def publish(topic, *args, **kwargs):
    if type(topic) != unicode:
        raise TypeError("URIs must be unicode - got {} instead".format(type(topic)))

?

@meejah
Copy link
Contributor

meejah commented Sep 15, 2015

Yes, I think if an error could reasonably be seen by a user, it should be a "real" Exception (or RuntimeError). Or, as above, sometimes the built-in exceptions are the right answer (but, IMO, one should reach for RuntimeError/Exception first).

assert is for telling fellow programmers: "when I wrote this, I thought X could/would never really happen, and if it does this code will Very Likely do the wrong thing".

@hawkowl
Copy link
Contributor

hawkowl commented Sep 16, 2015

Yeah, I think that's a good approach. Exception or such for things which are expected (users doing the wrong config is "expected" in this scenario), and asserts are for "here is our assumptions about our own code". For example, we might make an assertion that the connection is connected in loseConnection.

@oberstet
Copy link
Contributor Author

@meejah @hawkowl ok: https://github.com/tavendo/AutobahnPython/blob/master/DEVELOPERS.md#use-of-assert-vs-exceptions

I leave this issue open for actually cleanup up the code according to above guide line.

@oberstet oberstet changed the title Use assert instead of raise idiom Use of assert/exceptions should follow the code style guide Sep 16, 2015
@oberstet
Copy link
Contributor Author

related: #486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants