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.

Sign up for GitHub

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

Jump to bottom

Add TASTy reflect constructors #5438

Merged
merged 1 commit into from
Dec 18, 2018
Merged

Add TASTy reflect constructors #5438

merged 1 commit into from
Dec 18, 2018

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Nov 13, 2018 •
edited
Loading

No description provided.

@nicolasstucki nicolasstucki self-assigned this Nov 13, 2018
@nicolasstucki nicolasstucki force-pushed the add-tasty-constructors branch 7 times, most recently from 55be7b7 to 19d0ae9 Compare November 19, 2018 09:50
This was referenced Nov 21, 2018
Merged
Closed
@liufengyun
Copy link
Contributor

It would be nice if we can prioritize this, as is required to implement ScalaTest macros.

For example, to support a > b where a could be any type, we need support dynamic selection & construction.

@liufengyun liufengyun mentioned this pull request Nov 30, 2018
Closed
66 tasks
@nicolasstucki
Copy link
Contributor Author

We may not need the constructors to perform those operation. We do have some tricks to encode those with quotes and splices.

@liufengyun liufengyun mentioned this pull request Dec 3, 2018
Closed
@nicolasstucki nicolasstucki mentioned this pull request Dec 7, 2018
Merged
@nicolasstucki nicolasstucki force-pushed the add-tasty-constructors branch 4 times, most recently from 07db2e3 to 852b423 Compare December 10, 2018 10:07
@nicolasstucki nicolasstucki force-pushed the add-tasty-constructors branch 6 times, most recently from 19f89e7 to f206a33 Compare December 10, 2018 20:26
@nicolasstucki nicolasstucki mentioned this pull request Dec 10, 2018
Merged
@nicolasstucki nicolasstucki force-pushed the add-tasty-constructors branch 6 times, most recently from 3cdb02b to 7b16047 Compare December 17, 2018 14:03
@nicolasstucki nicolasstucki force-pushed the add-tasty-constructors branch 6 times, most recently from 48c6d83 to 9304a3a Compare December 18, 2018 08:10
@nicolasstucki
Implement TASTy reflect constructors
5419a4c 
* Add tree constructors (apply in their module)
* Add tree copier (copy in their module)
* Add missing parameter of Term.Repeated
* Add Term.Ref as a supertype of Term.Ident and Term.Select
* Rename Project by Projection
liufengyun
liufengyun approved these changes Dec 18, 2018
View reviewed changes
Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not against merging this and iterate fast. I would first focus on constructors for terms and make sure they satisfy the needs (as the use cases for type trees and patterns are not clear).

compiler/src/dotty/tools/dotc/tastyreflect/PatternOpsImpl.scala
case lit: tpd.Literal => tpd.cpy.Literal(original)(lit.const)
case ref: tpd.RefTree if ref.isTerm => tpd.cpy.Ref(original.asInstanceOf[tpd.RefTree])(ref.name)
case ths: tpd.This => tpd.cpy.This(original)(ths.qual)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need apply and copy for Value? I have the impression that constructors are usually concrete.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I am considering splitting the Value type into more concrete types for literals and term references

library/src/scala/tasty/reflect/TreeOps.scala
@@ -114,6 +131,8 @@ trait TreeOps extends Core {

val DefDef: DefDefModule
abstract class DefDefModule {
def apply(symbol: DefSymbol, rhsFn: List[Type] => List[List[Term]] => Option[Term])(implicit ctx: Context): DefDef
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have API to create symbols in order to use this constructor? The same for ValDef.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. Needs to be added.

library/src/scala/tasty/reflect/TreeOps.scala

// TODO def apply(qualifier: Term, name: String, signature: Option[Signature])(implicit ctx: Context): Select

def copy(original: Tree)(qualifier: Term, name: String)(implicit ctx: Context): Select
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For later: some documentation is needed to specify what is going to happen if name is overloaded.

@nicolasstucki nicolasstucki merged commit b654af7 into scala:master Dec 18, 2018
@nicolasstucki nicolasstucki deleted the add-tasty-constructors branch December 18, 2018 10:09
@biboudis
Copy link
Contributor

@nicolasstucki the description of this merged PR says that it is based on a PR that is not merged yet. Can you summarize the situation very briefly?

@nicolasstucki
Copy link
Contributor Author

I removed the commits from the other PR (i.e. this is not based on that PR). I forgot to remove the comment.

@biboudis
Copy link
Contributor

Merci!

@biboudis biboudis added this to the 0.12 Tech Preview milestone Jan 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liufengyun liufengyun liufengyun approved these changes

Assignees

@liufengyun liufengyun

Labels
None yet
Projects
None yet
Milestone
0.12 Tech Preview
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@nicolasstucki @liufengyun @biboudis