Mantis Bugtracker
  

Viewing Issue Simple Details Jump to Notes ] View Advanced ] Issue History ] Print ]
ID Category Severity Reproducibility Date Submitted Last Update
0003448 [Squeak] Compiler minor always 04-09-06 22:51 09-07-10 20:03
Reporter nicolas cellier View Status public  
Assigned To
Priority normal Resolution open  
Status new   Product Version 3.9
Summary 0003448: OutOfScopeNotification is caught by ParagraphEditor evaluateSelection
Description Thus if you evaluate in a text pane:

  Object compile: 'outOfScopeExample
    true ifTrue: [| i | i := 1].
    ^i'.
  ^Object new perform: #outOfScopeExample

you will get 1 as result.
but if you put same code in a file and file it in via FileBrowser menu for example, you get a SyntaxError.
Also you cannot modify the method in a browser, load it from Monticello, etc...
That is an unconsistent behavior.
Additional Information tim argued that above code is equivalent to
  outOfScopeExample
    | i |
    true ifTrue: [i := 1].
    ^i
because there is no proper closure in default compiler.
That is correct, but then does this suggest the OutOfScopeNotification should be removed everywhere ?
Or shouldn't we better enforce the notification everywhere ?

If we want to enforce the notification, we must remove the exception handling from ParagraphEditor. But we first have to change the method MessageAsTempNode>>#scope to return 0 or receiver scope, instead of -1. This will cure the inspector and debugger contextual evaluations.

But then, there might be other problems in script evaluation i am not aware of, that did maybe justify the exception handling... so my help stops here.
Attached Files  Compiler-Tests-OutOfScope.1.cs [^] (1,767 bytes) 04-09-06 22:51
 Compiler-Patch-OutOfScope.1.cs [^] (5,072 bytes) 04-14-06 22:04
 Compiler-Patch-OutOfScope.2.cs [^] (5,073 bytes) 04-21-06 23:14
 Compiler-Patch-OutOfScope.3.cs [^] (5,121 bytes) 04-24-06 05:08

- Relationships

- Notes
(0004722 - 294 - 300 - 300 - 300 - 300 - 300)
andreas
04-10-06 09:50

I think the real issue is that it's perfectly fine for ParagraphEditor to catch OutOfScope during *compilation* but not when *evaluating* the code. In other words, Compiler>>evaluate: should really be broken into two parts, compilation (which handles OutOfScope) and evaluation (which doesn't).
 
(0004748 - 1160 - 1256 - 1256 - 1256 - 1256 - 1256)
nicolas cellier
04-14-06 22:12

Andreas, very nice way of not breaking anything.
In this case, the change should be easy to make...
Well, except these details:
- #tallySelection call a non implemented method (at least not implemented in 3.9a 7020 and old compiler)
- #compileSelectionFor:in: (which is called by #debugIt) explicitely call Compiler instead of evalutaorClass new

So, two minor fixes are also required.
Also note that debugIt and tallySelection do not seem to write in the change log...

A small thing remain: eventually change cannot be replayed from a change list browser, but that is also the case for contextual DoIt, and maybe we can forget about it.

There is also another thing i do not like: FakeClassPool>>#adopt:
Changing a global var for passing information underground is not something very pure!
It is not tailored for multi-process for example... we'd rather not follow the path of VW making multi-process UI, or we'll bump into this one.

The patch i propose is not very light: it adds two methods to compiler (to separate generate... and execute...), and should be ported to new compiler also. Maybe code could be better factored, but it works...
 
(0004767 - 194 - 212 - 212 - 212 - 212 - 212)
nicolas cellier
04-21-06 23:12

Oops! my first patch has a typo bug (context instead of aContext).
That prevent some contextual evaluation to work...

So, throw away Compiler-Patch-OutOfScope.1.cs and rather take version 2.
 
(0004783 - 523 - 571 - 699 - 699 - 699 - 699)
nicolas cellier
04-24-06 05:08

as stated in http://bugs.impara.de/view.php?id=3498, [^]
there is another problem in ParagraphEditor>>compileSelectionFor:in:
it does not set method selector to #DoIt, and then this make MessageTally fail.

Note that this method was used only by debugIt menu, and that this menu work because it really install the method in the MethodDictionary with the side effect of setting method selector and class...

But since i want to use compileSelectionFor:in: in another place, i need a patch.
So i provide patch version 3...
 
(0010532 - 2144 - 2616 - 2616 - 2616 - 2616 - 2616)
nicolas cellier
04-12-07 16:35

Stephane Rollandin reported on squeak-dev "MessageAsTempNode>>scope question" that this one does not work, while it should:
[| a |
    a := 1.
         Transcript show:
        (Compiler new evaluate: '1+a' in: thisContext to: nil)
] fork

Compiler-Patch-OutOfScope.2.cs does not fix this, neither does .3.cs.
They are cosmetic changes just in the hope to not break anything...
But they only cure one side effect, not the main problem.
So i went back to first proposition in additionnal information.

MY CONCLUSIONS ARE:

1) What is the meaning of the OutOfScopeNotification? to prevent access of Block local variable outside the block.
2) a temporary in the scope of thisContext is not OutOfScope,
   however, it will raise an OutOfScopeNotification
   because MessageAsTempNode>>scope does return -1.
3) an error handling in ParagraphEditor suppress the OutOfScopeNotification in all cases (for thisContext scope and for true outOfScope cases), but only in interactive mode from within a text pane, not in batch or fork mode.
4) contextual variables might be used outside interactive text pane mode, as shown by Stephane example (also imagine a workspace contents filed in in batch)
5) IT IS GOOD TO ALWAYS ENFORCE RULE 1),
   even if byte code implementation enables such access.
   (and we shouldn't care of implementation, just formal definition of language)
   I do not see valuable exceptions when we should allow local variable access out of scope.
6) Putting a Compiler error handling in ParagraphEditor would make sense if error handling is to interact with user
   (because we know compiler is used in interactive mode).
   Here, it is the contrary, we blindly accept when in interactive mode and refuse to compile in batch mode...

ACTION REQUEST:

Given 1) 2) 3) and 4) indeed MessageAsTempNode>>scope should return 0.

Anyone has an objection ? (case when a MessageAsTempNode could be out of scope?)

Given 5) and 6), we should remove handling of the notification in ParagraphEditor.

Anyone has an objection ? (Good reason to access out of scope)
Anyone to check if any implication in Etoys or something?
 
(0013780 - 246 - 274 - 274 - 358 - 358 - 358)
FrankShearar
05-17-10 07:45
edited on: 05-17-10 07:47

With the fix for 0007532, this now returns nil. If you browse to Object>>outOfScopeExample in trunk, you'll see that the out of scope i is highlighted in red. If you edit (say by adding a trailing space) and recompile, you're told "out of scope".

 
(0013868 - 129 - 141 - 141 - 141 - 141 - 141)
nicolas cellier
09-07-10 20:03

The examples are solved by true closure indeed.

One question remain: why should ParagraphEditor catch OutOfScopeNotification ?
 

- Issue History
Date Modified Username Field Change
04-09-06 22:51 nicolas cellier New Issue
04-09-06 22:51 nicolas cellier File Added: Compiler-Tests-OutOfScope.1.cs
04-10-06 09:50 andreas Note Added: 0004722
04-14-06 22:04 nicolas cellier File Added: Compiler-Patch-OutOfScope.1.cs
04-14-06 22:12 nicolas cellier Note Added: 0004748
04-21-06 23:12 nicolas cellier Note Added: 0004767
04-21-06 23:14 nicolas cellier File Added: Compiler-Patch-OutOfScope.2.cs
04-24-06 05:08 nicolas cellier Note Added: 0004783
04-24-06 05:08 nicolas cellier File Added: Compiler-Patch-OutOfScope.3.cs
04-12-07 16:35 nicolas cellier Note Added: 0010532
05-17-10 07:45 FrankShearar Note Added: 0013780
05-17-10 07:47 FrankShearar Note Edited: 0013780
09-07-10 20:03 nicolas cellier Note Added: 0013868


Mantis 1.0.8[^]
Copyright © 2000 - 2007 Mantis Group
69 total queries executed.
41 unique queries executed.
Powered by Mantis Bugtracker