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

Inspector - misc #197

Merged
merged 3 commits into from
Oct 24, 2023
Merged

Inspector - misc #197

merged 3 commits into from
Oct 24, 2023

Conversation

vemv
Copy link
Member

@vemv vemv commented Oct 19, 2023

  • orchard.inspect: don't use pr-str over the main Value: being inspected.
    • All values are already formatted as strings, so this pr-str was redundant.
  • orchard.inspect: render non-accessible fields better.
    • If a given field cannot be inspected (because it's private and the JDK module system prevents opening it), we return the fixed symbol <non-inspectable value> for representing its value, clients being free to elide its rendering.
    • Now, for a given inspected Object (except Class objects), we return these sections, if present: Instance fields, Static fields ,Private instance fields, Private static fields.
    • For Class objects, we keep grouping the fields under a single Fields section.
  • orchard.inspect: render field names as symbols, not strings.

Cheers - V

@vemv vemv requested a review from bbatsov October 19, 2023 18:12
@vemv vemv force-pushed the inspect-misc branch 2 times, most recently from d532636 to 6b934f1 Compare October 19, 2023 18:53
@alexander-yakushev
Copy link
Member

orchard.inspect: don't render inaccessible fields anymore

So the inspector would no longer allow to descend into private fields? This is a huge usecase for the inspector, at least for me.

@alexander-yakushev
Copy link
Member

image Cross-module access was shut down in 17. So, 66% of users still benefit.

@vemv
Copy link
Member Author

vemv commented Oct 20, 2023

With the advent of JDK 21 and the practical deprecation of JDK8 (CIDER will do it; I hear it that even Clojure may join that trend), I suspect that that demographic will abruptly change.

This is a huge usecase for the inspector, at least for me.

I'd understand that, otoh predictably sooner or later you'll use JDK17+ for the vast majority of your CIDER sessions.

Note that with enrich-classpath, it's easier than ever to jump to a .java source so one can also learn about private fields in the source itself (and presumably java-mode or lsp-mode can help navigating a large .java file).

I do sympathize with wanting features to stay, but sadly sometimes the world around us moves forward 🌀

@bbatsov
Copy link
Member

bbatsov commented Oct 20, 2023

I'm strongly against making changes that would negatively impact JDK 8 users, given how conservative Clojure devs often are. Until JDK 8 is supported by Clojure (and used by so many people as indicated by last survey), we should do our best to support it as well.

@bbatsov
Copy link
Member

bbatsov commented Oct 20, 2023

So the inspector would no longer allow to descend into private fields? This is a huge usecase for the inspector, at least for me.

Btw, are we sure there's no way to implement a similar behavior for newer JDKs? I'm kind of worried how many features disappear (e.g. evaluation can't be interrupted, private fields can't be accessed, etc) as things are changed in the JDK.

@alexander-yakushev
Copy link
Member

alexander-yakushev commented Oct 20, 2023

and the practical deprecation of JDK8

Says who? :suspect:

@bbatsov
Copy link
Member

bbatsov commented Oct 20, 2023

(also - bigger changes of the breaking variety should probably have to wait until CIDER 2.0)

@alexander-yakushev
Copy link
Member

Btw, are we sure there's no way to implement a similar behavior for newer JDKs? I'm kind of worried how many features disappear (e.g. evaluation can't be interrupted, private fields can't be accessed, etc) as things are changed in the JDK.

I don't know of any other way but adding lots of --add-opens to the startup command. But I suspect I'll start doing that once I move onto newer JDK for local development.

Inability to interrupt threads is a big hit, yes. I tried looking into whether there are some leftovers for thread stopping in the JDK code that could be reenabled with an agent, for example, and couldn't find anything (but I didn't dig deep enough).

@alexander-yakushev
Copy link
Member

Besides – nobody seems to be embracing module separation except for JDK itself. Third-party libraries all use the default unnamed module, so their private fields remain to be accessible. It's only the JDK's own modules that we aren't able to crack open.

@vemv
Copy link
Member Author

vemv commented Oct 20, 2023

A middle ground surely is to keep trying to access fields, but hide the failed attempts.

@alexander-yakushev
Copy link
Member

alexander-yakushev commented Oct 20, 2023

I don't mind. I made it show the failed attempts to signal to the user why a certain field is not shown. If you think it's too jarring, then fine.

UPD: actually, I do mind, because the presence of a field is itself a useful piece of information. And being able to jump to the source isn't the same because: a) you need enrich-classpath which might not be always present; b) the source file can have a lot of javadocs and other cruft which makes quickly seeing which fields an object has quite difficult. c) if the class has parents, the inspector shows all fields at once while you have to navigate all parents through the source manually.

UPD2: the inspector could be grouping the private inaccessible fields together in the end of the list so that they don't bother so much but still remain visible.

@vemv vemv marked this pull request as draft October 20, 2023 09:27
@vemv
Copy link
Member Author

vemv commented Oct 21, 2023

UPD2: the inspector could be grouping the private inaccessible fields together in the end of the list so that they don't bother so much but still remain visible.

Let's go for this one. I'll update the PR soon.

btw, here's a toggler for .java source code (including docstrings) https://gist.github.com/vemv/7953ce6900fdf597e6199212752ecf32

…nspected

All values are already formatted as strings, so this `pr-str` was redundant.
@vemv vemv marked this pull request as ready for review October 23, 2023 21:16
@vemv
Copy link
Member Author

vemv commented Oct 23, 2023

We got this as requested!

Please check out the changelog for a detailed summary.

This is how things are looking now:

Objects

Screen Shot 2023-10-23 at 22 13 01

  • The main value is not a string anymore
  • field names are symbols now, not strings
  • There are up to 4 groups for fields, some of them possibly absent
  • Values for unaccessible (not private!) fields are rendered as literally nothing (while the field name remains being clickable)

Classes

Screen Shot 2023-10-23 at 22 04 51

  • Just one group for all fields

@alexander-yakushev
Copy link
Member

This looks gorgeous! Thanks for taking the time to resolve this in the best possible manner.

@bbatsov
Copy link
Member

bbatsov commented Oct 24, 2023

Looks great indeed. Well done!

@vemv
Copy link
Member Author

vemv commented Oct 24, 2023

Cheers 🙌

@vemv vemv merged commit 9cb236c into master Oct 24, 2023
@vemv vemv deleted the inspect-misc branch October 24, 2023 09:13
vemv added a commit to clojure-emacs/cider that referenced this pull request Oct 24, 2023
vemv added a commit to clojure-emacs/cider that referenced this pull request Oct 24, 2023
Those are conveyed by Orchard/cider-nrepl for representing values that cannot be inspected (i.e. private, inaccessible fields).

See clojure-emacs/orchard#197

Fixes #3542
vemv added a commit to clojure-emacs/cider that referenced this pull request Oct 24, 2023
vemv added a commit to clojure-emacs/cider that referenced this pull request Oct 24, 2023
Those are conveyed by Orchard/cider-nrepl for representing values that cannot be inspected (i.e. private, inaccessible fields).

See clojure-emacs/orchard#197

Fixes #3542
@alexander-yakushev
Copy link
Member

alexander-yakushev commented Oct 26, 2023

Wait, so you did remove accessing private fields? This is on Java8:
image

@vemv
Copy link
Member Author

vemv commented Oct 26, 2023

Hmm, I didn't mean to remove setAccessible.

Should be an easy fix

@alexander-yakushev
Copy link
Member

Thanks! ❤️

vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 26, 2023
Fixes an oversight made in #197
vemv added a commit that referenced this pull request Oct 27, 2023
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