Opened 3 years ago

Closed 3 years ago

#5561 closed bug (fixed)

assertion overriden by other exceptions

Reported by: MikolajKonarski Owned by:
Priority: normal Milestone:
Component: Compiler Version: 7.3
Keywords: Cc:
Operating System: Linux Architecture: x86_64 (amd64)
Type of failure: Incorrect result at runtime Difficulty:
Test Case: assert Blocked By:
Blocking: Related Tickets:

Description

The attached file, containing

main = let e1 i = throw Overflow
       in assert False (e1 5)

and compiled with

ghc --make -O1 -fno-ignore-asserts test.hs

produces

test: arithmetic overflow

and should produce

test: test.hs:25:11-16: Assertion failed

Works OK, if compiled with

ghc --make -O0 test.hs

Attachments (1)

test.hs (777 bytes) - added by MikolajKonarski 3 years ago.

Download all attachments as: .zip

Change History (8)

Changed 3 years ago by MikolajKonarski

comment:1 Changed 3 years ago by igloo

  • Resolution set to wontfix
  • Status changed from new to closed

comment:2 Changed 3 years ago by igloo

  • Resolution wontfix deleted
  • Status changed from closed to new

Sorry, misread it.

comment:3 Changed 3 years ago by igloo

I think the fix will be to use lazy in assertError:

assertError :: Addr# -> Bool -> a -> a
assertError str predicate v
  | predicate = lazy v
  | otherwise = throw (AssertionFailed (untangle str "Assertion failed"))

comment:4 Changed 3 years ago by igloo

We've been debating this on IRC. I think we're all agreed that the current behaviour is correct, in the sense that that's how imprecise exceptions work. But we could "fix" this case, as above, to behave as expected.

The "fixed" version would still be correct in the sense of imprecise exceptions. It would behave differently to user-defined functions that aren't similarly fixed, although you can already get differences depending on whether GHC decides to inline assertError or not.

Note that performance of optimised code won't generally suffer, as normally optimisations mean assert is disabled anyway.

Mikolaj and I think it is worth fixing this case. Duncan thinks it will just add to the confusion. Any other opinions?

comment:5 Changed 3 years ago by simonmar

Tricky one, but I think we should "fix" it with lazy as suggested, and put a comment pointing to this ticket next to the fix.

comment:6 Changed 3 years ago by simonpj

I agree with Simon. It's true that

(throw this && throw that)

should be allowed to return either exception this or that, but

assert False (throw this)

should really report the assertion failure, not throw the exception.

Putting an example like this with the modification to assertError, plus a pointer to the ticket, would be good. Ian can you do?

Simon

comment:7 Changed 3 years ago by igloo

  • Resolution set to fixed
  • Status changed from new to closed
  • Test Case set to assert

Done:

commit cc4774d8b1e2736c4b9171db05451ba6355c98b6
Author: Ian Lynagh <igloo@earth.li>
Date:   Wed Oct 19 22:23:19 2011 +0100

    If an assertion fails, through it rather than a deeper error; fixes #5561

    An expression like
        assert False (throw e)
    should throw the assertion failure rather than e
Note: See TracTickets for help on using tickets.