Modified StDebuggerActionModel to use Exception>>defaultDescription…

[emdonahue]

… as the debugger's window title rather than the context predicate. This brings the behavior of Exception signalIn: thisContext in line with Exception signalwith respect to having the same window title.

[StevenCostiou]

I think this commit highlights a problem but will break things.

First we should not use Exception>>defaultDescription but Exception>>description.
Some exceptions have customized descriptions, and do not use the default one (or complement it).
Calling Exception>>defaultDescription will break the DNU customized message for example.

Second, the context predicate uses information from the execution context to customize the string printed for the current context.
If you look at the implementors of printDescription there is more behavior than returning a printed exception.
The commit will break behavior in some cases.

I think the problem is that the context predicate does not account for doits methods such as in your example.
The printDescription method should be improved to account for it.

[emdonahue]

@StevenCostiou I take your point on Exception>>description.

Regarding printDescription, in which cases does it give correct behavior currently? Currently playground DoIts as well as unit tests are issues.

[StevenCostiou]

Hmm from what I can remember (this code is a couple of years old I think), it was very difficult to correctly display an accurate status string in the window title.
Very hard to obtain something satisfying.

Many cases are complicated to detect or even to define.
It is totally possible that some are not working in a satisfying way, like the doits from your example.

But for example, it works (or used to) for halts, because depending on how you hit a halt (from running code or from the debugger stepping) then the information you have at your disposal to detect that you should write "halt" is not the same.
The unit tests was also a struggle, because detecting a unit test context from the debugger is actually not uniform at all, and we have (or had) to check for many things in the context and in the exception. Do you have a running example of the problem with the tests that you mention?

The exception is not sufficient by itself for many cases, and replacing the printDescription by just a print of the exception is likely to make lots of other debugging context windows displaying odd or useless information.

The context predicate was introduced to "calculate" these cases and detect in which case we should use the exception, and in which we should extract other information from the context itself.

Perhaps it is not good code, but I think it could be improved to cover more cases.
Why not "integrating" your fix in the print description behavior?

[StevenCostiou]

Or perhaps the printDescription could be simplified or removed?
Maybe we should first define what are all the cases (i.e., what should that or that context print in the window title) with tests then make sure the retained solution covers everything?

[StevenCostiou]

Note: I see printDescription has some tests but 2 out of 3 are skipped.
I have no memory of that, nor why they were skipped.

[emdonahue]

@StevenCostiou I'd be happy to integrate it, I just need to better understand the functionality of printDescription.

The issue seems to be that StDebuggerActionModel>>contextSignalsAnException: checks whether a context is an exception context by literally looking at whether the receiver is Exception and the message is #signal, which will be false almost all the time you use Exception>>signalIn: since that sets the signalContext (which is what is checked) to whatever context you use, which is then extracted in computeInitialTopContext. Since the action model doesn't know that it's an exception context, it returns the wrong context predicate (not the error one) and that predicate doesn't use the exception's description in the window. Normal signal works because it sets its own context as the signalContext using thisContext inside the call to signal and then it is a signal send so it is checked explicitly later. Halt works because it's technically returning a non error context predicate but that predicate seem specially designed to handle Halt because the halt message is part of its hardcoded prefix text and it then prints the context, which is specially made for the halt message. Halt is almost the only thing that uses signalIn currently, which is probably why the issue hasn't come up.

An example of this in a unit test would just be to use Exception signalIn: thisContext in any unit test. I think that that code will raise the issue almost no matter where it is called. I think signalIn: is basically broken in this regard, except in the special case of Halt, which is the only place it is really used.

The reason this came up is that I was working on a contracts library for runtime dynamic type checking and I wanted to perform a typecheck on a method call (using MetaLinks) and then throw an error at the call site rather than in my type checking code, similar to the behavior of Halt. I started developing this in Pharo 6, where signalIn still worked, and just returned to it now to finally release it only to find that it no longer works in Pharo 11 due to the signalIn: issue.

As far as a fix, I wonder why Halt is handled specially outside of StDebuggerErrorContextPredicate. It seems like the action model needs to better detect exceptions triggered with signalIn, and then maybe just handle Halt along with those unless there's a reason to keep it out. Do you know why Halt is handled so specially? It seems like either Halt could be handled by the error context predicate or else the normal context predicate could be polymorphic about what actual error message it prints rather than hardcoding the context, but I don't know enough about why those two predicates exist separately.

[emdonahue]

@StevenCostiou The main special cases I see are Error, TestFailure, and Halt, with one predicate for each (the superclass predicate seems to be mostly a Halt predicate, although it also handles "postmortem," which I don't know what that refers to). Are there any other meaningfully different cases you can think of to handle & do you know what a post mortem is here?

[emdonahue]

@StevenCostiou I changed it to description and added a custom description to Halt that mimics what I understand to be the current Halt title behavior, although I'm not sure I understand why Halt prints the context only if it is not signaled directly. I also added test cases for the instances I was aware of. Can you think of any situations that will break this? The biggest question mark is postmortems, as I don't understand the conditions under which those arise. I am not currently using any of the logic in the ContextPredicates, so it is possible there may be an issue if topContext differs from signalContext, but I am not sure when that would happen. I don't know enough about the Exception handling system to be able to reason about the edge cases, so any breaking examples you can think of would be welcome.

[StevenCostiou]

We'll do a review with @adri09070 (sorry for the delay)

[adri09070]

I think that this is bad. #isKindOf: is costly and the behavior can be reproduced by calling StDebuggerContextPredicate>>#printDescriptionPrefixOn:. Using this method would also allow to handle post-mortems (which are just dead contexts: contexts that have already terminated in which we can't step anymore).

Overriding #description in the class Halt causes the debugger to display Halt when a breakpoint is hit, because the class Break inherits from Halt.

From looking at the code of StDebuggerActionModel>>#printHaltDescription:, I would say that the fact that "Halt in" not being displayed by the debugger when executing Halt now is a bug, so trying to reproduce it like that is not good.

[adri09070]

We can't throw away context predicates like this.

For example, it handles printing for post-mortems and halts.
DoIts are also handled by context predicates. DoIts are just contexts with an OupsNullException and that's why the debugger doesn't display them correctly after your changes.

Plus, what you do is done in StDebuggerErrorContextPredicate: it prints the description of the exception only if it the context is not a DoIt.
So, if I look at the problem from the beginning, the problem is simply that an StDebuggerErrorContextPredicate is not created because the debugger action model doesn't consider exceptions signaled via #signalIn: as exceptions.
As you said, this is because of the method:

StDebuggerActionModel>>#contextSignalsAnException
  ...
  and: [  aContext selector = #signal ] ]

So, I would propose as a fix to:

• remove Halt>>#description,
• reverse StDebuggerActionModel>>#statusStringForContext to what it was before the changes,
• in StDebuggerActionModel>>#contextSignalsAnException, change the line and: [ aContext selector = #signal ] ] to and: [ (#signal signalIn:) includes: aContext selector ] ]
• Then, in StDebuggerErrorContextPredicate>># printDescription, we would have to handle Halt just the same way as OupsNullException. To avoid using kindOf, you could define an extension method in the class Exception isDebuggerWhiteListed that would return false and that would be overriden by OupsNullException and Halt to return true...

[emdonahue]

Thanks for the comments. I think what is needed most is a set of additional test cases that clarify some of the intended behavior. Since I don't have a spec for the exception handling system, I can only code against guesses about what correct behavior should look like based on examples I find in the image, some of which seem like bugs, as you mention, and some of which I could not find in order to produce, such as any code that generates a post-mortem. Could you perhaps suggest some tests that clarify some of these issues and intended behaviors?

[adri09070]

I think you can add a test in which a debugger action model proceeds until it reaches a method with a Halt that uses #signal (such as 1 halt) and we could check that the description is the same as the one we would get with Halt if: [true]: "Halt in class>>method`

For post-mortems, it is really hard to get one and I can't think of an easy way to do it. Generally, it happens when we materialize a stack that had been previously fueled out in a file.
In a unit test, what you could do is create one "artificially". You could create a debugger action model on any session and store the context status string into a variable. Then, you could make the context post-mortem artificially by executing debuggerActionModel contextPredicate postMortem: true .
Then, you could check that the context status string is the same as the one you've stored into your variable but prefixed by [Post-mortem] .

I don't think there are more cases than these ones and the ones you already cover in your tests. I don't know what @StevenCostiou thinks

[emdonahue]

Thanks. I will try to get that working next time I have time to work on this. Would it be preferable to pull request just the tests, or is it preferred to have working code along with tests?

I'm trying to get my contracts/type checking library working, but it is considerably less useful than it could be if the exceptions it throws can't describe the type error.

[Ducasse]

@StevenCostiou @adri09070 So what is the conclusion?

[Ducasse]

I changed branch and rerun the tests

[emdonahue]

I was ultimately unable to understand the scope of the error handling system well enough to make the relevant changes and ended up having to abandon the project that depended on this functionality (contracts for runtime dynamic type checking that needed to work with exception objects). I am not currently working on it. Apologies.