Skip to content

Add constructors for types #6280

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

Closed
biboudis opened this issue Apr 10, 2019 · 20 comments
Closed

Add constructors for types #6280

biboudis opened this issue Apr 10, 2019 · 20 comments

Comments

@biboudis
Copy link
Contributor

Add all the constructors for Types here library/src/scala/tasty/reflect/TypeOrBoundsOps.scala

For testing we can create a TypeMap. Pass types deconstructing and constructing again and check the identity (note: check tests for TreeMap from library/src/scala/tasty/reflect/TreeUtils.scala, the test is tests/run/tasty-tree-map adapted from the corresponding one from the compiler)

@biboudis biboudis self-assigned this Apr 10, 2019
@nicolasstucki nicolasstucki changed the title Add constructors for constant types Add constructors for types Aug 1, 2019
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Aug 1, 2019
nicolasstucki added a commit to dotty-staging/dotty that referenced this issue Aug 5, 2019
@liufengyun
Copy link
Contributor

@nicolasstucki : you forget to make a PR? b379dd7

@nicolasstucki
Copy link
Contributor

@biboudis wants to do it

@fhackett
Copy link
Contributor

fhackett commented Jan 7, 2020

Hi, I'm trying to get started as a contributor and @nicolasstucki suggested I try working on this issue (see mention above). My focus is to enable macro developers to work with more complex local expressions than is currently possible at the Tasty level. See #7626 for my motivating example.

Since I'm new to this I'll try to document my progress in case anyone has comments / criticism / etc.

For starters, I'm aiming to get some tests written. I see the note that working based on TreeMap could be useful, but I also notice that that uses *.copy(original)(...) rather than typical constructor calls. Identity tests make sense for transformations, but for construction wouldn't it make more sense to test that we can splice the results of these constructors into generated code?

@nicolasstucki
Copy link
Contributor

Indeed, splicing the result seems to be the way to do it.

@fhackett
Copy link
Contributor

fhackett commented Jan 8, 2020

Progress update: I've caught some of the low-hanging fruit like ConstantType, AndType, OrType and TypeBounds. My changes are stored here: https://github.com/fhackett/dotty/tree/fhackett-work-on-6280

When trying to work with more interesting types like TypeLambda however I'm getting confused. The TypeLambda from TypesOrBounds seems to only refer to generic lambdas, while there is a different LambdaTypeTree that is used for actual type lambdas.

Can anyone clarify what the relationship is between Type and TypeTree? It's not clear to me from source code or usage right now.

@fhackett
Copy link
Contributor

@olhotak helped me understand the above question irl. I was also enlightened as to the value of the Dotty videos vs reading textual docs as I was originally doing. Thanks!

For today's update, I managed to get almost all of the Tasty type constructors that make sense implemented and tested.

Aside: turns out my implementation for TypeLambda constructor was actually correct, there was just a Dotty implementation quirk that means that an immediately-applied HKTypeLambda (with AppliedType, as opposed to AppliedTypeTree) seems to never exist "in nature" due to early simplification, so making one in Tasty causes many strange, broken results. I "fixed" my tests to use an AppliedTypeTree materialized via quotes instead.

Assuming I don't think of something new to do over night (and no-one comments anything big in that time), PR coming soon.

@nicolasstucki
Copy link
Contributor

A TypeTree represents user syntax for a Type. This implies that every TypeTree has a Type and a position in source. But the full structure of a Type does not necessarily map back to a TypeTree.

val a = List(2)
val b: List[Int] = List(2)

Both a and b will have the type List[Int] but only b will have the TypeTree representing the type in the source.

@fhackett
Copy link
Contributor

Having taken care of most of the simpler types (references aside), MethodType and PolyType remain. MethodType already has a constructor whereas PolyType does not.

I've experimented a bit and cannot find any sane way of checking these types against something else with =:= like in the PR above - turns out accessing the type of a method is prevented as such types are not considered stable.

That said, I have a plan to make these type constructors usable in practice (so, testable): implementing local method synthesis. The idea is to combine tpd.polyDefDef, which uses an existing symbol and its (method) type to generate a synthetic DefDef tree, with a synthetic symbol (given an appropriate parent derived from the Context owner) and a synthetic type.

With that feature, we can effectively test constructed MethodType and PolyType by using them to synthesize different kinds of local method. Bonus points for this being the feature that lead to me taking on this issue in the first place.

If no serious problems with this are exposed, I'll try putting together a second PR for this sometime next week.

@fhackett
Copy link
Contributor

I've started experimenting with the above, and I've run into something I can't make sense of.
Given a definition like this:

def foo : Int = 2

Every experiment I try shows that the type of foo is Int. Not a MethodType with no parameters, because that gives foo() instead.

But, when I try to generate a method symbol with type Int I get an error from TreeChecker.scala:345, which suggests that all DefDefs must have a MethodType.

Any hints on what I'm missing? Flags? Some other subclass of MethodType?

@fhackett
Copy link
Contributor

Never mind, found it by reading some of the namer's code. You need an ExprType.

@nicolasstucki
Copy link
Contributor

Yes, that is it. ExprType is also used for by-name parameters.

@nicolasstucki
Copy link
Contributor

nicolasstucki commented Jan 29, 2020

@fhackett
Copy link
Contributor

fhackett commented Jan 29, 2020

To add my thoughts: I'm hesitant to just write the apply methods and call it done as I've found these constructors quite opaque and confusing to work with [EDIT: for an end-user, given all the special nestings of ThisType, TypeRef, etc... required to just make one path work].

I'll spend some time from my next Dotty work slice seeing if I can build a more useable API for these, similar to how term-wise we have Term.select to build Select nodes.

@nicolasstucki
Copy link
Contributor

Maybe I add those. It is true that Thier API might be tricky.

@fhackett
Copy link
Contributor

@nicolasstucki Seems we have been bitten by the ambiguity / incompleteness of only communicating via Github issues. Here is my attempt at ironing out any uncertainty that has leaked into this thread, hopefully it (and my little edit above, as I realised my wording was ambiguous) helps.

I wasn't super sure if people were interested in working out that API, so I didn't immediately jump into it. That said, since you seem interested in getting this closed I'm fine with finishing up this issue.
On the other hand, if you have a particular interest in doing this part that's fine too.

Let me know if I should keep this on my TODO list for next week or leave it to you.

@nicolasstucki
Copy link
Contributor

I will move things around in #8138.

@fhackett
Copy link
Contributor

@nicolasstucki Thanks for the heads up. I'll take cover for a moment.

(given lack of a specific answer, I assume you imply that yes, I should keep this issue on my TODO)

@nicolasstucki
Copy link
Contributor

The big refactoring has been done

@fhackett
Copy link
Contributor

fhackett commented Feb 11, 2020

For the last two types remaining (TypeRef, ThisType), I experimented with a more coherent interface than just giving them apply methods... and failed to come up with anything useful that wouldn't be way too complicated and fragile (though I did learn a lot about compiler internals).

PR coming up with a simple implementation and some tests.

@fhackett
Copy link
Contributor

Never mind. It became more complicated than I expected.

The problem I ran into was that macros are not shown things like Denotation and Name, which means that no matter what this will be a lossy interface. Then, we have the fact that constructing ThisType and TypeRef inside the compiler seems to be an involved process anyway, and just doing something tends to break things.

The interface I was thinking of adds Symbol.classType, an accessor that generates TypeRefs for the class the symbol refers to (currently not possible if you just have the symbol). Then, you also have Type.member(String) which allows generating type projections. You might also need variations for term members vs type members.

With that kind of interface users shouldn't have to worry (too much) about breaking Dotty, while being able to project types as needed.

I'll just leave this here for comment as I'm not sure I've covered all the cases.

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

No branches or pull requests

4 participants