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 Throwable and Error #916

Closed
gavinking opened this issue Jan 27, 2014 · 38 comments
Closed

add Throwable and Error #916

gavinking opened this issue Jan 27, 2014 · 38 comments

Comments

@gavinking
Copy link
Member

We have decided, finally, to add Throwable and Error classes, which erase to java.lang.Throwable and java.lang.Error, thus reflecting the model that exists in the Java VM.

@ghost ghost assigned gavinking Jan 27, 2014
@tombentley
Copy link
Member

@gavinking I think we can close this

@gavinking
Copy link
Member Author

I agree. Thanks Tom!

@gavinking gavinking reopened this Jul 4, 2014
@gavinking
Copy link
Member Author

OK, reopening this, because somehow it didn't occur to anybody to think through what this was going to mean on a JS VM. FFS!

@gavinking
Copy link
Member Author

So, AFAICT, there is no way in JS to portably detect something like an out of memory or stack overflow. So we're going to have to totally rethink this shit from the ground up. AFAICT we're going to have to remove c.l.Error because it simply can't be implemented in JS.

@gavinking
Copy link
Member Author

After reviewing the situation with JS, it seems to me that what we need to do is:

  1. remove Error,
  2. make AssertionError a direct subtype of Throwable (though perhaps the native impl could descend from j.l.Exception),
  3. treat all native exceptions in the JS VM as direct subtypes of Throwable (this means wrapping them in the catch block).

@tombentley
Copy link
Member

So, AFAICT, there is no way in JS to portably detect something like an out of memory or stack overflow.

I'm not convinced this is a huge problem. We're not going to have Ceylon exception types for those things, so we're not claiming they're necessarily represented via the exception hierarchy (or represented at all). I think Error still makes sense as an abstraction. And I think there are places where Ceylon can use Error (for example when we detect extra cases of enumerated types).

Anyway, going along with your proposal for now:

  • Is Throwable still enumerated, presumably not?
  • I think it makes more sense for the native impl of Ceylon AssertionError to extend j.l.Error, then it is at least unchecked.

@FroMage
Copy link
Member

FroMage commented Jul 7, 2014

I don't think that we should get rid of Error just because native JS will never throw it. Ceylon code will throw it and Java code too, so I don't see the point of removing something that's useful.

@gavinking
Copy link
Member Author

No, Error is not useful, because:

  1. Nobody should ever write catch (Error e) to begin with. At best there exist very rare situations in which catch (Throwable e) is correct.
  2. That's doubly true when Error doesn't even identify a well-defined set of cases which constitute errors, and would behave differently on different platforms.
  3. It's even true if Error only identified certain Ceylon-specific cases which actually belong to a broader category of things which aren't all Errors.

So we need to remove it. We really didn't think this one through at all.

@gavinking
Copy link
Member Author

  • Is Throwable still enumerated, presumably not?

No.

  • I think it makes more sense for the native impl of Ceylon AssertionError to extend j.l.Error, then it is at least unchecked.

OK, sure, I agree, that's a good reason.

@tombentley
Copy link
Member

ceylon.language.meta.declaration.ModelError currently extends Error. I guess we could make it extend Throwable directly.

@tombentley
Copy link
Member

I suppose Throwable is no longer abstract (because there's no need for it to be, now it's no longer enumerated) and it enhances interop if it's not.

@tombentley
Copy link
Member

I suppose Throwable is no longer abstract

Actually we can't make Throwable concrete while maintaining the current erasure rules (if we did then Throwable(msg).string would evaluate as "java.lang.Throwable: ...").

@tombentley
Copy link
Member

I thought it might make sense if we made the native Throwable extend j.l.Error (we'd then instantiate Ceylon Throwables, but catch j.l.Throwable). However that doesn't work because we need to declare a addSuppressed() method to make it visible to the Ceylon typechecker, but javac won't let us because it's final. So maybe Throwable ought to be abstract after all.

@gavinking
Copy link
Member Author

Actually it might be interesting to make Throwable sealed.

@tombentley
Copy link
Member

In this case, sealed in its own wouldn't solve the erasure problem, it would have to be sealed abstract.

Anyway, how would making it sealed work when there would blatantly be other types which extend Throwable but are not in the language module.

@gavinking
Copy link
Member Author

Well, it all depends upon what you think sealed means :)

@tombentley
Copy link
Member

sealed means you can't instantiate it outside the module, right? Therefore you can't subclass it either. Since you're proposing to "treat all native exceptions in the JS VM as direct subtypes of Throwable" aren't you just going to make the sealed look a bit ridiculous? Likewise j.l.Error would appear to violate the sealed flag. I don't see that making Throwable actually solves any problem that we have but it certainly seems to add new ones. Or have I got this completely wrong?

@gavinking
Copy link
Member Author

sealed means you can't instantiate it outside the module, right?

Well, it certainly means you can't do it in Ceylon code. Not much we can do to enforce sealed in native code.

@tombentley
Copy link
Member

But what problem does using sealed solve?

The problem is that on the one hand I can't erase c.l.Throwable to j.l.Throwable (because you'll see from the Throwable.string that something fishy is going on), on the other hand I can't make c.l.Throwable extend any JVM throwable class because some of the methods which c.l.Throwable needs to define are actually final in the JVM superclass.

@gavinking
Copy link
Member Author

But what problem does using sealed solve?

No specific problem, it just tells people that exceptions they define in Ceylon should be either Exceptions or they should be AssertionErrors. Because I've no clue what good reason they could possibly have for directly extending Throwable.

@tombentley
Copy link
Member

I'm going to make Throwable abstract due to the erasure problem mentioned above.

I can't make Throwable sealed because we can't put Ceylon annotations on native implementations right now (ceylon/ceylon.language#408 / ceylon/ceylon-compiler#470).

The metamodel throws ceylon.language.meta.declaration.ModelError, as mentioned above. This really needs to be some kind of error, rather than an Exception (we're not expecting people to catch this). Sometimes it's thrown because an interlying Java API throws a checked exception: We probably don't want to discard the stacktrace in that case, but AssertionError (the only alternative to Exception) doesn't have a cause (and it doesn't really need one for its primary use). I wonder if we need another subclass of Throwable, say, ImplementationError which doesn't represent an error of the Ceylon user, but rather an implementation error in ceylon.language or the compiler. Thoughts?

@FroMage
Copy link
Member

FroMage commented Jul 10, 2014

You mean like Error? ;)

@tombentley
Copy link
Member

Well, we could just throw j.l.Error (or some java subclass) because it's not like this is recoverable, or means the same thing across platforms. That's also fine. I just don't want to be accused of not thinking it through.

@tombentley
Copy link
Member

  • Let's frig the model loader to make Throwable appear sealed without needing to read the annotation.
  • Let's get rid of the Ceylon ModelError and make iy a Java class which directly extends j.l.Error, on the basis that it's a platform-level exception. No one would ever need to catch this exception specificly in Ceylon code.

tombentley added a commit to ceylon/ceylon.language that referenced this issue Jul 17, 2014
…ve that

directly extends Throwable (in ceylon), but actually extends (j.l.Error in the JVM runtime).
tombentley added a commit to ceylon/ceylon.language that referenced this issue Jul 17, 2014
tombentley added a commit to ceylon/ceylon.language that referenced this issue Jul 17, 2014
tombentley added a commit to ceylon/ceylon.language that referenced this issue Jul 17, 2014
@tombentley
Copy link
Member

I have pushed a throwable branch to ceylon.language with the new Throwable, without Error etc. I now need @chochos to implement the ceyon-js part of this, both the language module part and also this bit

treat all native exceptions in the JS VM as direct subtypes of Throwable (this means wrapping them in the catch block).

Once we have that I we can merge the branch and I can push my compiler changes.

@tombentley tombentley assigned chochos and unassigned gavinking Jul 17, 2014
@gavinking
Copy link
Member Author

Great, thanks.

@chochos
Copy link
Member

chochos commented Jul 18, 2014

ModelError is not in this throwable branch... is it because it's older than that file, or because it's removed in the branch?

chochos added a commit to ceylon/ceylon.language that referenced this issue Jul 18, 2014
chochos added a commit to ceylon/ceylon-js that referenced this issue Jul 18, 2014
chochos added a commit to ceylon/ceylon-js that referenced this issue Jul 18, 2014
@chochos
Copy link
Member

chochos commented Jul 18, 2014

OK, changes pushed to throwable branches of ceylon.language and ceylon-js. Feel free to merge.

@tombentley
Copy link
Member

@chochos

When I try to build a dist using your language module and js compiler it dies like this:

 [java] Exception in thread "main" java.lang.IllegalArgumentException: Invalid Ceylon language module source ../ceylon.language/src/ceylon/language/meta/declaration/ModelError.ceylon or ../ceylon.language/runtime-js/ceylon/language/meta/declaration/ModelError.ceylon
 [java]     at com.redhat.ceylon.compiler.js.Stitcher.compileLanguageModule(Stitcher.java:97)
 [java]     at com.redhat.ceylon.compiler.js.Stitcher.stitch(Stitcher.java:178)
 [java]     at com.redhat.ceylon.compiler.js.Stitcher.main(Stitcher.java:208)

@chochos
Copy link
Member

chochos commented Jul 18, 2014

Ah, so ModelError is indeed gone... You'll have to edit MASTER.js and remove that class from the list of compiled sources (sorry I don't think I'll have time today to fix it myself)

@chochos
Copy link
Member

chochos commented Jul 18, 2014

Btw MASTER.js is in ceylon.language/runtime-js

tombentley added a commit that referenced this issue Jul 18, 2014
tombentley added a commit to ceylon/ceylon-compiler that referenced this issue Jul 18, 2014
tombentley added a commit to ceylon/ceylon-compiler that referenced this issue Jul 18, 2014
tombentley added a commit to ceylon/ceylon-compiler that referenced this issue Jul 18, 2014
tombentley added a commit to ceylon/ceylon-compiler that referenced this issue Jul 18, 2014
tombentley added a commit to ceylon/ceylon-compiler that referenced this issue Jul 18, 2014
@tombentley
Copy link
Member

OK, all those branches etc have been merged. Could @gavinking review before closing this issue?

@gavinking
Copy link
Member Author

@tombentley Well there's just one thing wrong with this. Consider:

    try {
        throw AssertionError("oops");
    }
    catch (Error error) {
        print(error.message);
    }

This code prints "oops". But according to the definition of AssertionError in Ceylon, AssertionError isn't an instance of java.lang.Error.

So I think we need some special stuff in catch (Error error) to filter out AssertionErrors.

@gavinking
Copy link
Member Author

Even worse, this code results in a backend error:

    try {
        throw AssertionError("oops");
    }
    catch (Error error) {
        //error.printStackTrace();
        print(error.message);
    }
    catch (AssertionError ae) {
        print("ok");
    }

@tombentley
Copy link
Member

I guess some reordering of catch clauses or inserting of regrowing clauses could fix that.

@chochos
Copy link
Member

chochos commented Jul 18, 2014

um but Error doesn't even exist anymore?

@tombentley
Copy link
Member

He means java lang Error, I think

@gavinking
Copy link
Member Author

Closing, after opening ceylon/ceylon-compiler#1741.

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

4 participants