Skip to content

Rework TastyInspector API to allow inspection of all files #10792

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

Merged

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Dec 14, 2020

An alternative to #10777

Change the API to allow inspection of all trees at the same time.

- class MyInspector extends scala.tasty.inspector.TastyInspector:
-    def processCompilationUnit(using Quotes)(root: quotes.reflect.Tree): Unit = 
-      println(tree.show(using quotes.reflect.Printer.TreeStructure))
- 
- new MyInspector().inspectTastyFiles(tastyFiles)

+ class MyInspector extends scala.tasty.inspector.Inspector:
+   def inspect(using Quotes)(tastys: Tasty[quotes.type]): Unit =
+     for tasty <- tastys do 
+       println(tasty.ast.show(using quotes.reflect.Printer.TreeStructure))
+    
+ TastyInspector.inspectTastyFiles(tastyFiles)(new MyInspector)

Porting scala3doc is left for a future PR.

@nicolasstucki
Copy link
Contributor Author

This is the API that everyone has been asking for. Now we can inspect all TASTy files sources at once.

@abgruszecki could you have a look at the scala3doc side?

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from 7524970 to 780d890 Compare December 15, 2020 10:16
@nicolasstucki nicolasstucki added this to the 3.0.0-RC1 milestone Jan 14, 2021
@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch 6 times, most recently from 5e32f93 to 8baf71e Compare January 18, 2021 10:37
@nicolasstucki nicolasstucki added the release-notes Should be mentioned in the release notes label Jan 18, 2021
@nicolasstucki nicolasstucki marked this pull request as ready for review January 18, 2021 12:43
Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

Overall LGTM, I have only one small comment.

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from 8baf71e to 854d5ef Compare January 21, 2021 09:50
@nicolasstucki
Copy link
Contributor Author

@abgruszecki I added a Tasty interface to avoid all the (_,..) in for (_, tree) <- tastys do ... . It will also be useful if in the future if we what to extend this interface, maybe add a jpath method or something else we have not thought off yet.

@abgruszecki
Copy link
Contributor

It looks good. Having the Quotes instance inside Tasty shouldn't hurt.

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from a7e2628 to 5575ea2 Compare January 21, 2021 12:57
Copy link
Contributor

@abgruszecki abgruszecki left a comment

Choose a reason for hiding this comment

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

I think we could use a test that implements Inspector#inspect, uses the outer Quotes to retrieve a symbol, and compares it with the symbol of the nested ast - mostly as a sanity check that the types work out. Otherwise, LGTM.

@nicolasstucki
Copy link
Contributor Author

Could you give me an outline of the test you have in mind?

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from 5575ea2 to e190f91 Compare January 21, 2021 13:56
@nicolasstucki
Copy link
Contributor Author

@abgruszecki after our discussion about the current tests is it ok to merge this PR?

@abgruszecki
Copy link
Contributor

@nicolasstucki yep, no issue on that end.

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from e190f91 to 9ec2464 Compare January 25, 2021 14:34
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.

LGTM


/** Called after all compilation units are processed */
protected def postProcess(using Quotes): Unit = ()
object TastyInspector:
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: what about just call it Tasty? The rationale is to avoid the conceptual link with Inspector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We already have trait called Tasty that means something different. Maybe we need another name.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then it's fine to keep the current name.

@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from 9ec2464 to 795e465 Compare January 27, 2021 07:53
@nicolasstucki nicolasstucki force-pushed the rework-tasty-inspector-api branch from 795e465 to e903941 Compare January 27, 2021 12:18
@nicolasstucki nicolasstucki merged commit 3568ecd into scala:master Jan 28, 2021
@nicolasstucki nicolasstucki deleted the rework-tasty-inspector-api branch January 28, 2021 06:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants