[Pharo-project] Undo compiler temporary variable handling
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables? 1) In various code bases I see an increasing number of temp variables that shadow instance variables. This can lead to subtle bugs in various situations that are very hard to debug. Furthermore consider that the code that shadows instance variables in unlikely to load into any other Smalltalk. 2) In various code bases I see an increasing number of unused temporary variables. Instead of just leaving them in there it would be nice if the compiler removed them right away. Point 1 is a *major* annoyance, I already lost hours for that reason. Point 2 is not that important, as it has no intermediate consequences and can be deferred to code critics. I would need to write a new lint rule though. If I knew where these compiler changes were done, I would propose a fix for Pharo 1.0 and 1.1. Any pointers? Lukas -- Lukas Renggli http://www.lukas-renggli.ch
I agree with these points. Especially with point number 1. Cheers, Doru On 2 Mar 2010, at 21:48, Lukas Renggli wrote:
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables. This can lead to subtle bugs in various situations that are very hard to debug. Furthermore consider that the code that shadows instance variables in unlikely to load into any other Smalltalk.
2) In various code bases I see an increasing number of unused temporary variables. Instead of just leaving them in there it would be nice if the compiler removed them right away.
Point 1 is a *major* annoyance, I already lost hours for that reason.
Point 2 is not that important, as it has no intermediate consequences and can be deferred to code critics. I would need to write a new lint rule though.
If I knew where these compiler changes were done, I would propose a fix for Pharo 1.0 and 1.1. Any pointers?
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- www.tudorgirba.com "In a world where everything is moving ever faster, one might have better chances to win by moving slower."
Do you refer to the fact that we do not get this stupid pop-ups? there is preference for that I think. So we could set up by default. Did you see it? http://code.google.com/p/pharo/issues/detail?id=1224 Tell me if it helps you and sorry for the inconvenience incomplete progress is putting on you.
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables.
Yes this can be annoying we should find a solution in 1.1. http://code.google.com/p/pharo/issues/detail?id=2102
This can lead to subtle bugs in various situations that are very hard to debug. Furthermore consider that the code that shadows instance variables in unlikely to load into any other Smalltalk.
2) In various code bases I see an increasing number of unused temporary variables. Instead of just leaving them in there it would be nice if the compiler removed them right away.
http://code.google.com/p/pharo/issues/detail?id=2103
Point 1 is a *major* annoyance, I already lost hours for that reason.
Point 2 is not that important, as it has no intermediate consequences and can be deferred to code critics. I would need to write a new lint rule though.
If I knew where these compiler changes were done, I would propose a fix for Pharo 1.0 and 1.1. Any pointers?
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
2010/3/2 Stéphane Ducasse <stephane.ducasse@inria.fr>:
Do you refer to the fact that we do not get this stupid pop-ups? there is preference for that I think. So we could set up by default. Did you see it? Â Â Â Â http://code.google.com/p/pharo/issues/detail?id=1224
Tell me if it helps you and sorry for the inconvenience incomplete progress is putting on you.
Hi Steph. It's not a question of preference. It's a question of cross-dialect compatibility and introducing future obscure bugs in Pharo. I'm in the process of debugging Compiler/Decompiler mismatch, and I can tell you the code is complex enough without shadowed variable. I'm quite sure you can get large breakage of debugger tool if you ever allow shadowed tempVars (say as a copiedValue defined inan optimized block). Cheers
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables.
Yes this can be annoying we should find a solution in 1.1. Â Â Â Â http://code.google.com/p/pharo/issues/detail?id=2102
This can lead to subtle bugs in various situations that are very hard to debug. Furthermore consider that the code that shadows instance variables in unlikely to load into any other Smalltalk.
2) In various code bases I see an increasing number of unused temporary variables. Instead of just leaving them in there it would be nice if the compiler removed them right away.
    http://code.google.com/p/pharo/issues/detail?id=2103
Point 1 is a *major* annoyance, I already lost hours for that reason.
Point 2 is not that important, as it has no intermediate consequences and can be deferred to code critics. I would need to write a new lint rule though.
If I knew where these compiler changes were done, I would propose a fix for Pharo 1.0 and 1.1. Any pointers?
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Hi Steph. It's not a question of preference.
you got me wrong. I introduced a preference based on hernan changes (for TDD) to avoid to have popup showing up all the time and I was wondering if this is this change that introduces this bug. | a | a + 1 was not related to shadowing but to variable usage. So now it may have also cancel the identification of shadow and this is what I asked lukas to check.
It's a question of cross-dialect compatibility and introducing future obscure bugs in Pharo.
I do not see why this is cross dialect compatibility? Is shadowing variable is not allowed in Smalltalk? In VisualWorks you get a pop up but you can define it. Now that the tool helps you to avoid them when you create them is different than the semantics level.
I'm in the process of debugging Compiler/Decompiler mismatch, and I can tell you the code is complex enough without shadowed variable. I'm quite sure you can get large breakage of debugger tool if you ever allow shadowed tempVars (say as a copiedValue defined inan optimized block).
probably.
Cheers
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables.
Yes this can be annoying we should find a solution in 1.1. http://code.google.com/p/pharo/issues/detail?id=2102
This can lead to subtle bugs in various situations that are very hard to debug. Furthermore consider that the code that shadows instance variables in unlikely to load into any other Smalltalk.
2) In various code bases I see an increasing number of unused temporary variables. Instead of just leaving them in there it would be nice if the compiler removed them right away.
http://code.google.com/p/pharo/issues/detail?id=2103
Point 1 is a *major* annoyance, I already lost hours for that reason.
Point 2 is not that important, as it has no intermediate consequences and can be deferred to code critics. I would need to write a new lint rule though.
If I knew where these compiler changes were done, I would propose a fix for Pharo 1.0 and 1.1. Any pointers?
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
I introduced a preference based on hernan changes (for TDD) to avoid to have popup showing up all the time and I was wondering if this is this change that introduces this bug.
| a | a + 1
That's a use of an 'undefined' temp. I haven't noticed that change yet, but indeed that could be problematic too (so we have a 3rd point). Lint already spots that issue today.
was not related to shadowing but to variable usage. So now it may have also cancel the identification of shadow and this is what I asked lukas to check.
When was this change done? Does anybody know? If so, I could look at the diff and propose a change.
It's a question of cross-dialect compatibility and introducing future obscure bugs in Pharo.
I do not see why this is cross dialect compatibility? Is shadowing variable is not allowed in Smalltalk?
ANSI doesn't say anything, so shadowing is probably fine. Generally, I am all for shadowing variables and assignments to arguments, but then the tools should not break. Also it should be consistent, because currently in some cases shadowing is disallowed (instance variables cannot shadow other instance variables, temporary variables and arguments cannot shadow other temporaries and arguments) and in some it is allowed (temps can shadow instance variables, class variables can shadow globals).
In VisualWorks you get a pop up but you can define it. Now that the tool helps you to avoid them when you create them is different than the semantics level.
Yes, that's cool (the user is always warned) and it is applied consistent across all types of variables (even instance variables that shadow other instance variables are possible). I think that's the ideal solution. Lukas -- Lukas Renggli http://www.lukas-renggli.ch
I did the following check. We should really get rid of this compiler. In 1.1 and I think that this is the wrong version of the changes that was introduced because the interface sucks ================= Parser warningAllowed: true Object subclass: #Zork instanceVariableNames: 't' zork | t | t := 1. I get t appears to beunused in this method.Ok to remove it? Really? Now indeed my changes (my responsibility of using hernan change) Parser warningAllowed: false Object subclass: #Zork instanceVariableNames: 't' zork | t | t := 1. gets the variable t shadows the instance variables. So for Pharo we get simply for now issue Parser warningAllowed: true to get it to the "normal" behavior In 1.0 ================= Parser warnUser but we have the same silly broken pop up. Now I checked and in 3.9 ================= we got 'Name is already defined.' so it seems that this is not the warning preference that are responsible of the problem. I will ask marcus if he remembers which compiler changes may have Stef
I did the following check. We should really get rid of this compiler.
Yes, but somebody has to write a new compiler first. The one in the image is the only one that works.
So for Pharo we get simply for now issue Parser warningAllowed: true to get it to the "normal" behavior
That's Pharo 1.1?
In 1.0 =================
Parser warnUser
but we have the same silly broken pop up.
Yeah, but that is better than accidently shadowing variables.
we got 'Name is already defined.' so it seems that this is not the warning preference that are responsible of the problem.
You lost me. :-) Lukas
I will ask marcus if he remembers which compiler changes may have Stef
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
-- Lukas Renggli http://www.lukas-renggli.ch
lukas can youreread because I tried to improve thte explanation.
In 1.1 and I think that this is the wrong version of the changes that was introduced because the interface sucks ================= Parser warningAllowed: true
Object subclass: #Zork instanceVariableNames: 't'
zork
| t | t := 1.
I get "t appears to beunused in this method.Ok to remove it?"
Now with Parser warningAllowed: false
Object subclass: #Zork instanceVariableNames: 't'
zork
| t | t := 1. the variable t shadows the instance variable without noticed
So for Pharo we get simply for now issue Parser warningAllowed: true to get it to the "normal" behavior.
In 1.0 ================= The interface changes -> Parser warnUser but we have the same silly broken pop up.
Now I checked and in 3.9 =================
we got 'Name is already defined.' which is the correct behavior so it seems that this is not the warning preference that are responsible of the problem.
Hi, I committed the change Compiler-JorgeRessia.141 Basically the problem was that the interactive mode was not taken into account in the Encode and BytecodeEncoder >> bindTemp: method. bindTemp: name "Declare a temporary; error not if a field or class variable or out-of-scope temp. Read the comment in Encoder>>bindBlockArg:within: and subclass implementations." self supportsClosureOpcodes ifFalse: [^super bindTemp: name]. scopeTable at: name ifPresent: [:node| "When non-interactive raise the error only if it is a duplicate" (node isTemp or: [requestor interactive]) ifTrue:[^self notify:'Name is already defined'] ifFalse:[self warnAboutShadowed: name]]. ^self reallyBind: name We tested it with Lukas and is working fine. However, we should see that there are no further side effects. Cheers, Jorge On Wed, Mar 3, 2010 at 9:37 AM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
lukas can youreread because I tried to improve thte explanation.
In 1.1 and I think that this is the wrong version of the changes that was introduced because the interface sucks ================= Parser warningAllowed: true
Object subclass: #Zork    instanceVariableNames: 't'
zork
   | t |    t := 1.
I get "t appears to beunused in this method.Ok to remove it?"
Now with    Parser warningAllowed: false
Object subclass: #Zork    instanceVariableNames: 't'
zork
   | t |    t := 1.  the variable t shadows the instance variable without noticed
So for Pharo we get simply for now issue Parser warningAllowed: true to get it to the "normal" behavior.
In 1.0 ================= The interface changes -> Parser warnUser but we have the same silly broken pop up.
Now I checked and in 3.9 =================
we got  'Name is already defined.' which is the correct behavior so it seems that this is not the warning preference that  are responsible of the problem.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
you rock guy! **thanks** a lot! Stef On Mar 3, 2010, at 11:52 AM, Jorge Ressia wrote:
Hi,
I committed the change Compiler-JorgeRessia.141
Basically the problem was that the interactive mode was not taken into account in the Encode and BytecodeEncoder >> bindTemp: method.
bindTemp: name "Declare a temporary; error not if a field or class variable or out-of-scope temp. Read the comment in Encoder>>bindBlockArg:within: and subclass implementations." self supportsClosureOpcodes ifFalse: [^super bindTemp: name]. scopeTable at: name ifPresent: [:node| "When non-interactive raise the error only if it is a duplicate" (node isTemp or: [requestor interactive]) ifTrue:[^self notify:'Name is already defined'] ifFalse:[self warnAboutShadowed: name]]. ^self reallyBind: name
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
Cheers,
Jorge
On Wed, Mar 3, 2010 at 9:37 AM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
lukas can youreread because I tried to improve thte explanation.
In 1.1 and I think that this is the wrong version of the changes that was introduced because the interface sucks ================= Parser warningAllowed: true
Object subclass: #Zork instanceVariableNames: 't'
zork
| t | t := 1.
I get "t appears to beunused in this method.Ok to remove it?"
Now with Parser warningAllowed: false
Object subclass: #Zork instanceVariableNames: 't'
zork
| t | t := 1. the variable t shadows the instance variable without noticed
So for Pharo we get simply for now issue Parser warningAllowed: true to get it to the "normal" behavior.
In 1.0 ================= The interface changes -> Parser warnUser but we have the same silly broken pop up.
Now I checked and in 3.9 =================
we got 'Name is already defined.' which is the correct behavior so it seems that this is not the warning preference that are responsible of the problem.
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Jorge Ressia wrote:
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
I can't check right now, but I suspect it is the cause of this side-effect - you cannot use the same block variable name in the same method. E.g. foobar collectionA do: [:each | each foo]. collectionB do: [:each | each bar]. I get a syntax error on the second "each". Is this considered shadowing? -- Yanni
On Wed, Mar 3, 2010 at 7:58 AM, Yanni Chiu <yanni@rogers.com> wrote:
Jorge Ressia wrote:
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
I can't check right now, but I suspect it is the cause of this side-effect - you cannot use the same block variable name in the same method. E.g.
foobar collectionA do: [:each | each foo]. collectionB do: [:each | each bar].
I get a syntax error on the second "each". Is this considered shadowing?
No, it's not shadowing in the closure compiler. Are you in an old image? If not and you're using the closure compiler then there's a bug somewhere. My image does not warn. Again, I need to spend some time bringing the various closure compilers up-to-date, which I should have time to do real soon now. best Eliot
-- Yanni
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Yanni I imagine that you are right. Jorge I got problem with colorPaletteForDepth: depth extent: chartExtent I think that we should recompile the image. I should have thought about that. I could remove the last change for the moment. Stef On Mar 3, 2010, at 4:58 PM, Yanni Chiu wrote:
Jorge Ressia wrote:
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
I can't check right now, but I suspect it is the cause of this side-effect - you cannot use the same block variable name in the same method. E.g.
foobar collectionA do: [:each | each foo]. collectionB do: [:each | each bar].
I get a syntax error on the second "each". Is this considered shadowing?
-- Yanni
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Fixed in Compiler-JorgeRessia.142 The behavior should be the same as before removing the interactive validation. I recompiled the image and it worked fine. Lukas tried it out with some monticello test packages and worked too. Cheers, Jorge On Wed, Mar 3, 2010 at 6:34 PM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
Yanni
I imagine that you are right. Jorge I got problem with
    colorPaletteForDepth: depth extent: chartExtent
I think that we should recompile the image. I should have thought about that. I could remove the last change for the moment.
Stef
On Mar 3, 2010, at 4:58 PM, Yanni Chiu wrote:
Jorge Ressia wrote:
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
I can't check right now, but I suspect it is the cause of this side-effect - you cannot use the same block variable name in the same method. E.g.
foobar    collectionA do: [:each | each foo].    collectionB do: [:each | each bar].
I get a syntax error on the second "each". Is this considered shadowing?
-- Yanni
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
youpi! I integrate it now then. Stef On Mar 3, 2010, at 8:00 PM, Jorge Ressia wrote:
Fixed in Compiler-JorgeRessia.142
The behavior should be the same as before removing the interactive validation.
I recompiled the image and it worked fine.
Lukas tried it out with some monticello test packages and worked too.
Cheers,
Jorge
On Wed, Mar 3, 2010 at 6:34 PM, Stéphane Ducasse <stephane.ducasse@inria.fr> wrote:
Yanni
I imagine that you are right. Jorge I got problem with
colorPaletteForDepth: depth extent: chartExtent
I think that we should recompile the image. I should have thought about that. I could remove the last change for the moment.
Stef
On Mar 3, 2010, at 4:58 PM, Yanni Chiu wrote:
Jorge Ressia wrote:
We tested it with Lukas and is working fine. However, we should see that there are no further side effects.
I can't check right now, but I suspect it is the cause of this side-effect - you cannot use the same block variable name in the same method. E.g.
foobar collectionA do: [:each | each foo]. collectionB do: [:each | each bar].
I get a syntax error on the second "each". Is this considered shadowing?
-- Yanni
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Do you refer to the fact that we do not get this stupid pop-ups? there is preference for that I think. So we could set up by default. Did you see it? Â Â Â Â http://code.google.com/p/pharo/issues/detail?id=1224
Tell me if it helps you and sorry for the inconvenience incomplete progress is putting on you.
I don't know why this was changed? Probably point 1 is an unintended side-effect of point 2? I like the progress that Pharo 1.0 is doing. All in all it is a huge progress every week. It is only this tiny detail that I find should be undone :-)
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables.
Yes this can be annoying we should find a solution in 1.1. Â Â Â Â http://code.google.com/p/pharo/issues/detail?id=2102
I think this has to be changed as soon as possible, e.g. for Pharo 1.0 final. If somebody pointed me to where this was changed I could even assemble a fix. Point 2 could be a preference (or a lint rule). That doesn't hurry at all and is fine for Pharo 1.1 (or later). Lukas -- Lukas Renggli http://www.lukas-renggli.ch
Thanks lukas I think that we will have to have a look at the changes made to the compiler in 1.0. ;-/ I will wait that marcus shows up but indeed we should fix that Stef
Do you refer to the fact that we do not get this stupid pop-ups? there is preference for that I think. So we could set up by default. Did you see it? http://code.google.com/p/pharo/issues/detail?id=1224
Tell me if it helps you and sorry for the inconvenience incomplete progress is putting on you.
I don't know why this was changed? Probably point 1 is an unintended side-effect of point 2?
I like the progress that Pharo 1.0 is doing. All in all it is a huge progress every week. It is only this tiny detail that I find should be undone :-)
Before going to Pharo 1.0 final, could we please undo the compiler changes that handle the temporary variables?
1) In various code bases I see an increasing number of temp variables that shadow instance variables.
Yes this can be annoying we should find a solution in 1.1. http://code.google.com/p/pharo/issues/detail?id=2102
I think this has to be changed as soon as possible, e.g. for Pharo 1.0 final. If somebody pointed me to where this was changed I could even assemble a fix.
Point 2 could be a preference (or a lint rule). That doesn't hurry at all and is fine for Pharo 1.1 (or later).
Lukas
-- Lukas Renggli http://www.lukas-renggli.ch
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
Name: Compiler-marcus.denker.66 Author: marcus.denker Time: 25 August 2008, 2:48:04 pm UUID: 1ebf7e7c-ca64-4294-bc55-f89fdcdd1a50 Ancestors: Compiler-marcus.denker.65 -0007136: [BUG] LinkedList add:after: fails to update lastLink -0007156: Behavior>>#initialize sends #superclass: which can cause VM crash - delete #isValidWonderlandTexture - fix TClassAndTraitDescription>>#methods to return the methods of a class or trait - add #localMethods to return methods defined in a class, not included by a trait. -7147 Parser-removeUnusedTemps -0006503: RunArray cant be created through #new: Now I have to work on sometihng else. But what we could do it do download some of the old images and check until we find the potential changes Stef
I haven't looked deep enough to find the actual change, but looked for where the warning should be triggered. The method warnAboutShadowed: is called from bindTemp: when you shadow an instance variable, but the warning is only written to the transcript. Adrian On Mar 3, 2010, at 09:13 , Stéphane Ducasse wrote:
Name: Compiler-marcus.denker.66 Author: marcus.denker Time: 25 August 2008, 2:48:04 pm UUID: 1ebf7e7c-ca64-4294-bc55-f89fdcdd1a50 Ancestors: Compiler-marcus.denker.65
-0007136: [BUG] LinkedList add:after: fails to update lastLink -0007156: Behavior>>#initialize sends #superclass: which can cause VM crash - delete #isValidWonderlandTexture - fix TClassAndTraitDescription>>#methods to return the methods of a class or trait - add #localMethods to return methods defined in a class, not included by a trait. -7147 Parser-removeUnusedTemps -0006503: RunArray cant be created through #new:
Now I have to work on sometihng else. But what we could do it do download some of the old images and check until we find the potential changes
Stef
_______________________________________________ Pharo-project mailing list Pharo-project@lists.gforge.inria.fr http://lists.gforge.inria.fr/cgi-bin/mailman/listinfo/pharo-project
participants (8)
-
Adrian Lienhard -
Eliot Miranda -
Jorge Ressia -
Lukas Renggli -
Nicolas Cellier -
Stéphane Ducasse -
Tudor Girba -
Yanni Chiu