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 constructors for TypeOrBounds that are not refs or PolyType #7961

Merged
merged 1 commit into from Jan 17, 2020

Conversation

@fhackett
Copy link
Contributor

fhackett commented Jan 10, 2020

This PR addresses #6280, at least in part. Review by @nicolasstucki

Constructors added:

  • TypeBounds
  • ConstantType
  • AnnotatedType
  • AndType
  • OrType
  • TypeLambda
  • MatchType

All these types are tested by splicing them into the types of vals. Additional tests are included for constructors that were not added Refinement and AppliedType because they are related and would have been added were they not originally present.

Aside from constructors, TypeLambda.param(Int) was added in order to refer to the parameters of a TypeLambda during construction.

An accessor for internal.MatchCase[_,_] was also added to make the MatchType constructor useable in practice.

Things like TypeRef, TermRef, ThisType are not included because I could not think of a use for them that is not better (and more safely) served by less direct methods as I used in my test code.

PolyType is not included because to test that we would need need a way to synthesize DefDef declarations, which is not currently supported. ByNameType (or, ExprType) is also not testable for similar reasons.

Copy link

dotty-bot left a comment

Hello, and thank you for opening this PR! 🎉

All contributors have signed the CLA, thank you! ❤️

Have an awesome day! ☀️

@fhackett

This comment has been minimized.

Copy link
Contributor Author

fhackett commented Jan 10, 2020

Review by @nicolasstucki
(didn't catch that part before posting the PR, wonder if it works from comments?)
It seems not, even if I edit the original PR message. Sorry for the mention span.

@fhackett

This comment has been minimized.

Copy link
Contributor Author

fhackett commented Jan 10, 2020

Seems there's some kind of build issue... community build failed with Couldn't retrieve 'ch.epfl.lamp:dotty-doc:0.21.0-RC1'. and I have no idea if that has anything to do with this PR.

In any case, have a good week-end.

@nicolasstucki nicolasstucki self-assigned this Jan 11, 2020
@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Jan 13, 2020

I restarted the tests and it passed.

library/src/scala/tasty/reflect/TypeOrBoundsOps.scala Outdated Show resolved Hide resolved
).seal.asInstanceOf[quoted.Type[Any]]

'{
val x1 : ${x1T} = 1

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 14, 2020

Contributor

This is only testing that 1 is a subtype of x1T but not the other way around.
I propose to try a more direct approach with assetrions and without asInstanceOf like:

val x1T = ConstantType(Constant(1))
val x2T = OrType(ConstantType(Constant(1)), ConstantType(Constant(2)))
val x3T = AndType(ConstantType(Constant(3)), typeOf[Any])
val x4Lam =
      TypeLambda(
        List("A","B"),
        _ => List(TypeBounds(typeOf[Nothing], typeOf[Any]), TypeBounds(typeOf[Nothing], typeOf[Any])),
        (tl : TypeLambda) => tl.param(1))
val x5T =
      Refinement(
        typeOf[RefineMe],
        "T",
        TypeBounds(typeOf[Int], typeOf[Int]))
...

assert(x1T =:= '[1].unseal.tpe)
assert(x2T =:= '[1 | 2].unseal.tpe)
assert(x3T =:= '[1 & Any].unseal.tpe)
assert(x4T =:= '[[A, B] =>> B].unseal.tpe)
assert(x5T =:= '[RefineMe { type T = Int }].unseal.tpe)
...

Then you could also move the test to tests/run-macros

This comment has been minimized.

Copy link
@fhackett

fhackett Jan 16, 2020

Author Contributor

Good point. I've updated the tests to do this (code push coming soon).

I don't understand "move the test to tests/run-macros" though. That is already where the test is?

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 17, 2020

Contributor

I meant tests/pos-macros. Sorry.

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 17, 2020

Contributor

But it is ok like this

*/
def MatchCaseType(given Context): Type = {
import scala.internal.MatchCase
Type(classOf[MatchCase[_,_]])

This comment has been minimized.

Copy link
@nicolasstucki

nicolasstucki Jan 14, 2020

Contributor

This is too hacky. In the future, we won't have access to the classOf[MatchCase[_, _]]. This should probably be in the scala.internal interface (CompilerInterface).

This comment has been minimized.

Copy link
@fhackett

fhackett Jan 16, 2020

Author Contributor

This is interesting. Even in the tests, I had to use Type(classOf[List[_]]) in order to get the type of List.

Actually writing typeOf[List] gives me missing type parameter(s) for List. Writing typeOf[List[_]] satisfies Dotty but gives me AppliedType(List, TypeBounds(Nothing,Any)), which is not helpful.

Is this a bug or am I doing it wrong?

This comment has been minimized.

Copy link
@fhackett

fhackett Jan 16, 2020

Author Contributor

(leaving is as-is for now since it makes more sense to change this and the tests at the same time once we figure out the above question)

Constructors added:
- TypeBounds
- ConstantType
- AnnotatedType
- AndType
- OrType
- TypeLambda
- MatchType

All these types are tested by using `=:=` to compare them to equivalent type quotes. Additional tests are included for constructors that
were not added `Refinement` and `AppliedType` because they are related and would have been added were they not originally
present.

Aside from constructors, `TypeLambda.param(Int)` was added in order to refer to the parameters of a `TypeLambda` during
construction.

An accessor for `internal.MatchCase[_,_]` was also added to make the `MatchType` constructor useable in practice.

Things like `TypeRef`, `TermRef`, `ThisType` are not included because I could not think of a use for them that is not
better (and more safely) served by less direct methods as I used in my test code.

`PolyType` is not included because to test that we would need need a way to synthesize `DefDef` declarations, which is not
currently supported. `ByNameType` (or, `ExprType`) is also not testable for similar reasons.
@fhackett fhackett force-pushed the fhackett:fhackett-work-on-6280 branch from 72573e7 to 5b75df9 Jan 16, 2020
@nicolasstucki nicolasstucki merged commit 3768984 into lampepfl:master Jan 17, 2020
2 checks passed
2 checks passed
CLA User signed CLA
Details
continuous-integration/drone/pr Build is passing
Details
@nicolasstucki

This comment has been minimized.

Copy link
Contributor

nicolasstucki commented Jan 17, 2020

Thank you @fhackett

@fhackett fhackett deleted the fhackett:fhackett-work-on-6280 branch Jan 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.