Skip to content

Make tasty inspector receive tasty files directly #10061

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
Merged
merged 1 commit into from
Oct 28, 2020
Merged

Make tasty inspector receive tasty files directly #10061

merged 1 commit into from
Oct 28, 2020

Conversation

nicolasstucki
Copy link
Contributor

@nicolasstucki nicolasstucki commented Oct 21, 2020 •
edited
Loading

No description provided.

@nicolasstucki nicolasstucki self-assigned this Oct 21, 2020
@nicolasstucki nicolasstucki force-pushed the make-tasty-inspector-recive-tasty-files-directly branch 12 times, most recently from cc6037b to 31d5120 Compare October 22, 2020 15:08
@nicolasstucki
Make tasty inspector receive tasty files directly
06d988a 
We add ways to load a list of tasty files and all tasty files in a jar.
This change is aligned with the changes in progress on the `-from-tasty` compilation.

We remove the old way to load tasty file by passing the class name.
Loading by name had some unintended consecuences that made this API fragile.
 - Loaded Scala 2 classes by mistake (no tasty)
 - Loaded java classes by mistake (no tasty)
 - Load the wrong version of the tasty file because the classpath contains an older/newer version of that class (different tasty)
@nicolasstucki nicolasstucki force-pushed the make-tasty-inspector-recive-tasty-files-directly branch from 31d5120 to 06d988a Compare October 23, 2020 09:31
@nicolasstucki nicolasstucki marked this pull request as ready for review October 23, 2020 10:05
@nicolasstucki
Copy link
Contributor Author

nicolasstucki commented Oct 23, 2020 •
edited
Loading

@abgruszecki this change may affect the doc tool. Not sure if you already workaround this in your driver.

liufengyun
liufengyun reviewed Oct 27, 2020
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.

On the surface level, it seems the API is more complex to use.

I have never used this API. Maybe @abgruszecki and @bishabosha can share more ideas?

@bishabosha
Copy link
Member

@gzoller what do you think

@bishabosha
Copy link
Member

bishabosha commented Oct 27, 2020 •
edited
Loading

I guess this now makes it more clear how to use the API in some ways, as users must identify the top level class corresponding to the target being inspected, (if they are using inspector from a class obtained from .getClass, they will then need to do some manipulation if they want to inspect an object)

@bishabosha
Copy link
Member

can this work with relative paths?

abgruszecki
abgruszecki reviewed Oct 27, 2020
View reviewed changes
Copy link
Contributor

@abgruszecki abgruszecki left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The code LGTM, but I'm not sure why exactly we're making this change. Is it to avoid problems with loading .tasty files from unknown/unexpected jars on the classpath?

@nicolasstucki
Copy link
Contributor Author

Yes, it is to not accidentally load the wrong version of the class if another one is in the classpath. Or load from a class that has no TASTy.

@nicolasstucki
Copy link
Contributor Author

can this work with relative paths?

Yes it does. The stdlib tests use relative paths.

@nicolasstucki
Copy link
Contributor Author

if they are using inspector from a class obtained from .getClass, they will then need to do some manipulation if they want to inspect an object

The TASTy inspector is designed to take as input TASTy or jar files and inspect their TASTy. What you are describing is some kind of runtime reflection mixed with the tasty inspector, not really what this is designed for.

@bishabosha
Copy link
Member

bishabosha commented Oct 27, 2020 •
edited
Loading

looks good to me - I think its good to make it clear that this API is for file inspection and not class reflection

@romanowski romanowski mentioned this pull request Oct 27, 2020
Closed
liufengyun
liufengyun approved these changes Oct 28, 2020
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.

LGTM

@bishabosha bishabosha merged commit af70c5f into scala:master Oct 28, 2020
@bishabosha bishabosha deleted the make-tasty-inspector-recive-tasty-files-directly branch October 28, 2020 20:15
@bishabosha bishabosha mentioned this pull request Oct 28, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abgruszecki abgruszecki abgruszecki left review comments

@liufengyun liufengyun liufengyun approved these changes

Assignees

@liufengyun liufengyun

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nicolasstucki @bishabosha @liufengyun @abgruszecki