OutOfScopeNotification

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

OutOfScopeNotification

Nicolas Cellier
About http://bugs.squeak.org/view.php?id=3448, the visible
consequences are fixed by introduction of closure, but...

Can someone explain why should ParagraphEditor & al catch
OutOfScopeNotification at all ?

Nicolas

Reply | Threaded
Open this post in threaded view
|

Re: OutOfScopeNotification

Eliot Miranda-2


On Tue, Sep 7, 2010 at 1:07 PM, Nicolas Cellier <[hidden email]> wrote:
About http://bugs.squeak.org/view.php?id=3448, the visible
consequences are fixed by introduction of closure, but...

Can someone explain why should ParagraphEditor & al catch
OutOfScopeNotification at all ?

Looking at the Mantis case I'd say it was obsolete and ideally we would nuke the entire class and its uses, but we need to change the part of Encoder>>#encodeVariable:sourceRange:ifUnknown: that says

  (varNode isTemp and: [varNode scope < 0]) ifTrue:
  [^OutOfScopeNotification signal
ifTrue: [action value]
ifFalse: [self notify: 'out of scope']].

to "do the right thing".  This isn't the right thing:

  (varNode isTemp and: [varNode scope < 0]) ifTrue:
  [^action value].

Because for an out-of-scope temp this will cause a raise of an UndeclaredVariableWarning, which is wrong, which if uncaught will result in a notification of "Undeclared", which is misleading.

Instead one fix might be to change Encoder>>encodeVariable: name to something like
Encoder>>encodeVariable: name
^self encodeVariable: name
sourceRange: nil
ifOutOfScope:
[:nodeOrNil|
nodeOrNil
ifNil: [self undeclared: name]
ifNotNil: [self assert: (nodeOrNil isTemp and: [nodeOrNil scope < 0]).
    requestor notify: (nodeOrNil isArg ifTrue: ['block arg'] ifFalse: ['temp']),
' is out of scope']]

rename Encoder>>#encodeVariable:sourceRange:ifUnknown: to Encoder>>#encodeVariable:sourceRange:ifOutOfScope: and have it pass in either nil or the out-of-scope temp.

What would you do?

best,
Eliot
 

Nicolas




Reply | Threaded
Open this post in threaded view
|

Re: OutOfScopeNotification

Nicolas Cellier
It depends how we tolerate shadowing...
For example, if we allow a temp shadowing an ivar, then Compiler shall
distinguish the cases :
- variable is used out of scope -> notify: if interactive
- variable has another external scope -> just warn:

In such case, if we refactor the class and remove the ivar, we should
better create a binding  in Undeclared rather than raising a
compilation Error.

If we do not tolerate shadowing, then maybe we can ignore the edge
case and always bark.

But there is the problem of Workspace.
In a Workspace, we should better tolerate temp shadowing workspace variables.
In effect, the compiler does not bark in following case even if we
remove OutOfScopeNotification handling from ParagraphEditor:
        a := 4.
        a > 0
                ifTrue: [^a]
                ifFalse:
                        [| a |
                        a := 1.
                        ^a].
However, this one:
        a := 4.
        a > 0
                ifFalse:
                        [| a |
                        a := 1.
                        ^a].
        ^a
will notify an ' out of scope  ->'

A bit weird...

Workspace concept can also be used for example for saving complex
Object graphs with messages reconstructing the graph and with
variables for handling cycles.
If we want to store blocks (a sortBlock for example) with local temps
without caring too much of workspace variable name conflicts, then
shadowing is attractive.
Next question is: can Encoder deal with outer scope of a shadowed ?

Nicolas

2010/9/8 Eliot Miranda <[hidden email]>:

>
>
> On Tue, Sep 7, 2010 at 1:07 PM, Nicolas Cellier
> <[hidden email]> wrote:
>>
>> About http://bugs.squeak.org/view.php?id=3448, the visible
>> consequences are fixed by introduction of closure, but...
>>
>> Can someone explain why should ParagraphEditor & al catch
>> OutOfScopeNotification at all ?
>
> Looking at the Mantis case I'd say it was obsolete and ideally we would nuke
> the entire class and its uses, but we need to change the part of
> Encoder>>#encodeVariable:sourceRange:ifUnknown: that says
>   (varNode isTemp and: [varNode scope < 0]) ifTrue:
>   [^OutOfScopeNotification signal
> ifTrue: [action value]
> ifFalse: [self notify: 'out of scope']].
> to "do the right thing".  This isn't the right thing:
>   (varNode isTemp and: [varNode scope < 0]) ifTrue:
>   [^action value].
> Because for an out-of-scope temp this will cause a raise of an
> UndeclaredVariableWarning, which is wrong, which if uncaught will result in
> a notification of "Undeclared", which is misleading.
> Instead one fix might be to change Encoder>>encodeVariable: name to
> something like
> Encoder>>encodeVariable: name
> ^self encodeVariable: name
> sourceRange: nil
> ifOutOfScope:
> [:nodeOrNil|
> nodeOrNil
> ifNil: [self undeclared: name]
> ifNotNil: [self assert: (nodeOrNil isTemp and: [nodeOrNil scope < 0]).
>     requestor notify: (nodeOrNil isArg ifTrue: ['block arg'] ifFalse:
> ['temp']),
> ' is out of scope']]
> rename Encoder>>#encodeVariable:sourceRange:ifUnknown:
> to Encoder>>#encodeVariable:sourceRange:ifOutOfScope: and have it pass in
> either nil or the out-of-scope temp.
> What would you do?
> best,
> Eliot
>
>>
>> Nicolas
>>
>
>
>
>
>