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

The inspector should not use unsafe storeString #115

Open
nicolas-cellier-aka-nice opened this issue Feb 8, 2024 · 3 comments
Open

The inspector should not use unsafe storeString #115

nicolas-cellier-aka-nice opened this issue Feb 8, 2024 · 3 comments

Comments

@nicolas-cellier-aka-nice
Copy link

nicolas-cellier-aka-nice commented Feb 8, 2024

Recipe for a disaster:

| window |
MCWorkingCopyBrowser open.
window := ToolSet inspect: DependentsFields.
window model selectionIndex: 95

What happens?

  • The MCWorkingCopyBrowser adds a bunch of MCWorkingCopy in the DependentsFields, so as to add itself as a dependent and get notified of some package change.
  • Since the DependentFields is quite populated, the DictionaryInspector omits most of the keys and replace them with ellipsis '...'
  • when we select the ellipsis (at index 95 at this time of writing), the inspector is trying to print something wth storeString
  • the VM gets overwhelmed, and in my case finish by freezing

If we user-interrupted soon enough, debugging the problem is already a challenge.

Even if we use debugIt instead of doIt in the above code snippet, Morphic stepping will freeze the UI soon after setting the selectionIndex inst. var. of the Inspector.

What are the problem with storeString?

  • it's verbose (instVarAt:put:)
  • it can work on simple trees, but will store the objects several times for a more general graph, and will run indefinitely if the object graph is cyclic
    Invoking storeString on a MCWorkingCopy, even if not a cyclic graph (I hope), is not a thing to do, because it can consume several Gbytes just with the business of printing the ancestry.
  • the storeString of complex objects cannot be evaluated because of intrinsic limitations of the language (number of literals in a single method, execution stack depth, ...).
  • it's neither of much use for a human interface, we ain't gonna decipher several Gbytes of instVarAt:put: instructions.

storeString is for simple objects at best with simple graphs, its simplistic nature is otherwise a failure.

An inspector aiming at some robustness shall not invoke storeString.

@timrowledge
Copy link

timrowledge commented Feb 8, 2024 via email

@LinqLover
Copy link
Contributor

My motivation to use storeString here was that you can directly copy it into the "inspect element ..." prompt where you have to provide a key, or use it in a manual self at: ... expression. I get the disadvantages of this approach though ...

@timrowledge
Copy link

timrowledge commented Feb 8, 2024 via email

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

No branches or pull requests

3 participants