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

fix(CtElementImpl#toString): fix toString for SniperPrinter #3147

Merged
merged 6 commits into from
Nov 4, 2019

Conversation

nharrand
Copy link
Collaborator

@nharrand nharrand commented Oct 8, 2019

Since recent changes, CtElement#toString() calls the default prettyprinter which can be a SniperPrinter.
AFAIU, this fails for two reasons:

  • toString() clone the element before printing it, and SniperPrinter use getRoleInCompilationUnit(). But getRoleInCompilationUnit() implement a Scanner that look for the exact element in its parent children. Of course the clone has a parent but is not among the children of its parent.
  • SniperPrinter require some SourceFragmentContext to be pushed on a stack to print an arbitrary element that is not a CompilationUnit. This must be initialized before.

This PR implement a POC that solves these two problems. But I am not sure if that's the direction we want to take. (Currently it makes an ugly check in toString()'s body to check what sort of printer we have, but it could be rewritten to be hidden inside SniperPrinter.)
WDYT?
@monperrus
@pvojtechovsky (if you have time) am I mistaken on the behavior of SniperPrinter?

@nharrand
Copy link
Collaborator Author

nharrand commented Oct 8, 2019

(Also, merges and fixes #3144 )

@monperrus
Copy link
Collaborator

Sounds like a good idea!

@pvojtechovsky
Copy link
Collaborator

I do not like idea to have such heavy implementation of toString which clones model and then optionally does heavy sniper printing. But you probably discussed that somewhere and probably see advantages.
The existence of non standard method toStringDebug() shows what is wrong. I think that every Java developer first expects that toString() behaves like toStringDebug() and nice and heavy printing is done by other method.
I have seen some PRs where developer called toString() ... expecting that result is fast and safe ... but it is not case of current toString calling clone and heavy pretty printer.

Anyway the solution of this PR looks good to me. 👍 I am just nor sure whether we can call SniperPrinter.super (see my comment in sources) of scan method in case we have no compilation unit (no source fragments). In such case it would be safer to create real DJPP (to call toStringDebug()), which will never by mistake call any SniperPrinter code.

@nharrand nharrand changed the title WiP: fix(CtElementImpl#toString): fix toString for SniperPrinter fix(CtElementImpl#toString): fix toString for SniperPrinter Oct 21, 2019
@nharrand
Copy link
Collaborator Author

Ok thanks for the feedback!

So, new proposal: I don't think it makes sense to apply preprocessor with the Sniper prettyprinter in toString(). In addition it is a bit ugly to do instanceofs in toString depending on the printer.

This PR now migrates this sort of mechanism to each prettyprinter implementation. And SniperJavaPrettyPrinter does not clone anymore. It just prints without modifying the element.

WDYT?

@monperrus monperrus merged commit 8aa8f0b into INRIA:master Nov 4, 2019
@monperrus
Copy link
Collaborator

Thanks a lot!

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

Successfully merging this pull request may close these issues.

3 participants