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

Allow anonymous class types to be referenced #865

Closed
gavinking opened this issue Dec 3, 2013 · 40 comments
Closed

Allow anonymous class types to be referenced #865

gavinking opened this issue Dec 3, 2013 · 40 comments
Assignees
Milestone

Comments

@gavinking
Copy link
Member

I thought there was already an issue for this, since it's an idea I had years ago. Given an object declaration like this:

shared class process  { ... }

We could let you refer to the type of process (the "anonymous" class) using the syntax \Iprocess.

@gavinking
Copy link
Member Author

So what I'm thinking of doing here is introducing a special type constructor, defined in the language spec, considered to belong to ceylon.language:

TypeOf<process> p = process;

For now this would just be for the type of anonymous classes. But in future we could use it for other useful things like TypeOf<this>.

@gavinking gavinking modified the milestones: 1.1.5, 1.5 Jan 5, 2015
@gavinking gavinking self-assigned this Jan 5, 2015
@gavinking
Copy link
Member Author

Hrm. Let me walk that back. The idea doesn't really work so well for nested objects, for example: Foo.TypeOf<bar>.

@lucaswerkmeister
Copy link
Member

Foo.TypeOf

I thought the syntax would have been TypeOf<Foo.bar> (and TypeOf<foo.Bar>)… but I don’t see the advantage of this. The parser still has to learn to accept lidentifiers as types.

@gavinking
Copy link
Member Author

@lucaswerkmeister Foo.bar is a static reference of type Foo.bar(Foo), not of type Foo.bar.

The intuition behind TypeOf has supposed to be that it resulted in the type of its argument expression.

gavinking added a commit that referenced this issue Jan 6, 2015
@gavinking
Copy link
Member Author

Alright, so I have an experimental implementation of this, based on the original suggestion of using \Iprocess, but it requires some changes in exactly when/where we eliminate object types, since the spec continues to say that these types are never inferred. I have to be sure those change didn't break anything.

@FroMage
Copy link
Member

FroMage commented Jan 7, 2015

Trying this with the compiler tests yields lots of errors. Apparently {1, null} has type {Integer|null+} for example. I also see {true|false+} where we had {Boolean+}. Also other errors that I can't quite diagnose yet. And that issue with Java enums.

@FroMage
Copy link
Member

FroMage commented Jan 7, 2015

So how exactly do we end up with a type pretending to be a Ceylon enum type for REPLACE_EXISTING? The model loader doesn't do that. Where does that type come from then?

@FroMage
Copy link
Member

FroMage commented Jan 7, 2015

Sorry the model loader does do that, just found where.

@FroMage
Copy link
Member

FroMage commented Jan 7, 2015

So how about I set isNamed(false) on the pretend type, so that you won't refer to it but use its extended type (which is the enum type)? Would that work?

@FroMage
Copy link
Member

FroMage commented Jan 7, 2015

Otherwise I'd have to add a new boolean to TypeDeclaration to avoid making it more expensive for every type to be translated to a Java type as we don't have any marker so I have to check if a type is a subtype of java.lang.Enum and I suppose skip anonymous types.

@gavinking
Copy link
Member Author

@FroMage And you can't use your underlyingType() stuff for this?

@gavinking
Copy link
Member Author

So to be honest there is something a little inconsistent about this. The type of the expression [true] is [\Itrue], but the type of the expression ArrayList<Boolean> { true } is ArrayList<Boolean>, not ArrayList<\Itrue>.

This is actually the behavior I want, since ArrayList<T> isn't covariant in T. But it's still a little strange.

@lucaswerkmeister
Copy link
Member

Perhaps we could say that anonymous class types are illegal as type arguments. So you can form \Itrue? (it’s legal within a union type), but not [\Itrue] (since that’s syntax sugar for type arguments to Tuple) nor \Itrue->String (same thing with Entry). I think that should cover all “useful” cases where you need them (but I’m probably missing something).

@FroMage
Copy link
Member

FroMage commented Jan 8, 2015

underlyingType is only for primitive types. If we start putting class names in there we have to define a format and it becomes a burden to maintain.

I think isNamed should work, because you have to avoid anonymous types that actually have no names, like anonymous object values, right? That's totally non-denotable, even with \i since the names are generated.

As for [true] being a visible type, am I supposed to make it visible via reified generics? Or use a denotable type? Would getDenotableType give me [Boolean]?

@FroMage
Copy link
Member

FroMage commented Jan 8, 2015

This is causing all sorts of issues. I now have a {foo|bar} value, which contains a union so is erased to Iterable<? extends Object> assigned to a {FooBarSuperClass} which is a Iterable<? extends FooBarSuperClass> so it's not compatible and we have to add new casting rules. Am I really supposed to reify {foo|bar} in reified generics or can I just add Unit.denotableType at lots of places?

@lucaswerkmeister
Copy link
Member

If we disallowed anonymous class types in type arguments, we’d never have to reify them, right? I don’t think we need to support {foo|bar}, I don’t see what it’s worth.

@quintesse
Copy link
Member

or can I just add Unit.denotableType at lots of places?

I tried putting it in makeJavaType() while trying to fix another case where that {true|false+} popped up and it broke a whole bunch of things, so I decided to only add it to the exact places that needed it.

@jvasileff
Copy link
Member

I don’t see what it’s worth.

Apologies if this is off base - I haven't been following this closely. But might it be useful to be able to work in a type safe manner with subsets of enum values? Something like:

abstract class Color(shared actual String string)
        of black | white | red | green | blue { }

...

alias FordColorOptions => Set<\Iblack>;
{Car*} filterByColor(FordColorOptions favorites) { }

@gavinking
Copy link
Member Author

But might it be useful to be able to work in a type safe manner with subsets of enum values?

Right.

@gavinking
Copy link
Member Author

I think isNamed should work, because you have to avoid anonymous types that actually have no names, like anonymous object values, right? That's totally non-denotable, even with \i since the names are generated.

Well, no, for ceylon enums, the type actually exists. There is an actual class true_ and an actual class false_.

This is causing all sorts of issues. I now have a {foo|bar} value, which contains a union so is erased to Iterable<? extends Object> assigned to a {FooBarSuperClass} which is a Iterable<? extends FooBarSuperClass>

I would not erase true|false to Object. I would erase it to Boolean.

@FroMage
Copy link
Member

FroMage commented Jan 12, 2015

Well, no, for ceylon enums, the type actually exists. There is an actual class true_ and an actual class false_.

Sure, I'm only proposing to set this to Java enums. Ceylon enums are named anonymous. Value objects are unnamed anonymous ;) So this is more like value objects.

I would not erase true|false to Object. I would erase it to Boolean.

Well, unless we want to make it generic, I suppose you need to give me a method on ProducedType that turns a union of enum value types into they enum type. Otherwise we'll fuck it up a dozen ways.

@FroMage
Copy link
Member

FroMage commented Jan 21, 2015

So I have some code which on master gives me:

+  [QualifiedTypeExpression] <ConstructorRef(String)> (56:37-56:73) => Constructor[ConstructorRef.ConstructorRef]
|  + ConstructorRef [Identifier] (56:60-56:73)
|  +  [InferredTypeArguments]
|  +  [QualifiedTypeExpression] <ConstructorRef(String)> (56:37-56:58) => Class[ConstructorRef]
|  |  + ConstructorRef [Identifier] (56:45-56:58)
|  |  +  [InferredTypeArguments]
|  |  + package [Package] <unknown> (56:37-56:43)
|  |  + . [MemberOp] (56:44-56:44)
|  + . [MemberOp] (56:59-56:59)

And on this issue's branch gives me:

+  [QualifiedTypeExpression] <ConstructorRef.ConstructorRef(String)> (56:37-56:73) => Constructor[ConstructorRef.ConstructorRef]
|  + ConstructorRef [Identifier] (56:60-56:73)
|  +  [InferredTypeArguments]
|  +  [QualifiedTypeExpression] <ConstructorRef(String)> (56:37-56:58) => Class[ConstructorRef]
|  |  + ConstructorRef [Identifier] (56:45-56:58)
|  |  +  [InferredTypeArguments]
|  |  + package [Package] <unknown> (56:37-56:43)
|  |  + . [MemberOp] (56:44-56:44)
|  + . [MemberOp] (56:59-56:59)

Notice the difference between ConstructorRef.ConstructorRef(String) (this branch) and ConstructorRef(String) (master), which means that Constructor instances leak in ProducedType which makes some things go barf. Is this intentional? Should I support that? Or is it a bug?

FroMage added a commit to ceylon/ceylon-compiler that referenced this issue Jan 21, 2015
FroMage added a commit to ceylon/ceylon-compiler that referenced this issue Jan 21, 2015
@FroMage
Copy link
Member

FroMage commented Jan 21, 2015

I've worked around it and made the JVM backend work after your changes. I've added Class.isJavaEnum for Java enums. I think we should start replacing the bunch of boolean fields on the model with int flags and bitfields to save on space, but I'll file that as another issue because it requires testing.

I am ready to merge this when the JS backend is ready too. @chochos?

@chochos
Copy link
Member

chochos commented Jan 21, 2015

So is the issue still just \Iprocess? I'll get on it...

@chochos
Copy link
Member

chochos commented Jan 22, 2015

This works locally but anonymous classes are not directly available even if the object is shared.

@chochos
Copy link
Member

chochos commented Jan 22, 2015

ok ready to merge in branch spec865 of ceylon/ceylon-js

@FroMage
Copy link
Member

FroMage commented Jan 22, 2015

I'll merge all branches then, thanks.

FroMage added a commit to ceylon/ceylon-compiler that referenced this issue Jan 22, 2015
FroMage added a commit to ceylon/ceylon-compiler that referenced this issue Jan 22, 2015
@FroMage
Copy link
Member

FroMage commented Jan 22, 2015

All merged, closing.

@FroMage FroMage closed this as completed Jan 22, 2015
@gavinking
Copy link
Member Author

Oh! So does that mean we're all happy with this? :-)

@gavinking
Copy link
Member Author

This works locally but anonymous classes are not directly available even if the object is shared.

@chochos I don't understand this comment?

@FroMage
Copy link
Member

FroMage commented Jan 22, 2015

Well, we're not at least on the subject of syntax. I can't find in which issue I proposed different syntax (perhaps I didn't) like I discussed with you, perhaps I should open a new issue?

@gavinking
Copy link
Member Author

@FroMage Don't open an issue unless you have an actual proposal :-)

@FroMage
Copy link
Member

FroMage commented Jan 22, 2015

I do and we actually discussed it.

@FroMage
Copy link
Member

FroMage commented Jan 22, 2015

Done: #1205

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

6 participants