pharo-spec / Spec

Spec is a framework in Pharo for describing user interfaces.
MIT License
61 stars 63 forks source link

`SpCodePresenter>>#evaluate:onCompileError:onError:` does not take into account additional bindings when evaluating an expression #1506

Closed adri09070 closed 4 months ago

adri09070 commented 8 months ago

When evaluating an expression, the code presenter does not consider the bindings of its interaction model:

SpCodePresenter>>#evaluate:onCompileError:onError:
    result := self interactionModel compiler
                    source: aString;
                    environment: self environment;
                    failBlock: [
                        self announcer announce:
                                (SpCodeEvaluationFailedAnnouncement newContent:
                                         aString).
                        ^ compileErrorBlock value ];
                    evaluate.

So, if an interaction model allows to add additional bindings, it leads to a popup because the variable is not recognized, even if the binding DOES exist in the interaction model:

image

The problem appears because, in a code presenter, SpCodeSelectionCommand>>#evaluate:andDo: calls SpCodePresenter>>#evaluate:onCompileError:onError: which does not take into account additional bindings.

*The problem DOES NOT appear in the playground though, and I can't figure out why because it does use additional bindings ... One lead that could be tracked is that it does not use the same command class (it overrides the context menu to use StEvaluateCommand instead, which still calls SpCodePresenter>>#evaluate:onCompileError:onError: in the end...)

Proposed fix: I think we should just provide the bindings from the interaction model to the compiler:

SpCodePresenter>>#evaluate:onCompileError:onError:
     result := self interactionModel compiler
                    source: aString;
                    environment: self environment;
                    bindings: self interactionModel bindings;
                    failBlock: [
                        self announcer announce:
                                (SpCodeEvaluationFailedAnnouncement newContent:
                                         aString).
                        ^ compileErrorBlock value ];
                    evaluate.
adri09070 commented 4 months ago

@StevenCostiou

MarcusDenker commented 4 months ago

the compiler is setup with a "requestor", see SpCodeInteractionModel>>#compiler

compiler
    "Provide a compiler set up on the current context/class/receiver"

    ^ self doItReceiver class compiler
          context: self doItContext;
          receiver: self doItReceiver;
          requestor: self

This makes the compiler add a scope for the requestor, soo OpalCompiler>>#buildOuterScope

buildOuterScope
    | newScope |

    newScope := self semanticScope.
    self needRequestorScope ifTrue: [
        "the requestor is allowed to manage variables, the workspace is using it to auto-define vars"
        newScope := (self compilationContext requestorScopeClass new
            requestor: self requestor) outerScope: newScope].

    self bindings ifNotNil: [
        "if we passed additional bindings in, setup a scope here"
        newScope := (OCExtraBindingScope  new
            bindings: self bindings) outerScope: newScope].

    ^newScope

So normally this should ask the requestor before the bindings.

I wonder why this does not work correctly in your case, maybe this is due to #needRequestorScope returning false?

adri09070 commented 4 months ago

This is exactly the cause, thanks

It's a shame that this is not documented somewhere