[BUG] Refactoring Browser

In a method like below:

  test
    | a |
    ^self check
      ifTrue: [a := #(3 4]]
      ifFalse: [nil].

choosing the 'Inline Temporary' refactoring for the assignment results in
this:

  test
    ^self check ifTrue: [] ifFalse: [nil]

This is a bug, isn't it?

(Dolphin 5.0.1)

"Andreas Timmermann" <[hidden email]> wrote in message news:<ahbo08$c5a$05$[hidden email]>...
Why do you think is a bug? what was the code you expected?

Federico

> > In a method like below:
> >
> >   test
> >     | a |
> >     ^self check
> >       ifTrue: [a := #(3 4]]
> >       ifFalse: [nil].
> >
> > choosing the 'Inline Temporary' refactoring for the assignment results
in
> > this:
> >
> >   test
> >     ^self check ifTrue: [] ifFalse: [nil]
> >
> > This is a bug, isn't it?
>
> Why do you think is a bug? what was the code you expected?

I would've expected this (don't forget about the ^)

test
  ^self check ifTrue: [#(3 4)] ifFalse: [nil]

"Andreas Timmermann" <[hidden email]> wrote in message
news:ahhf4n$kvr$04$[hidden email]...
The inline temporary is one of the "refactorings" that isn't checked as
thoroughly as the others since it is almost impossible to verify that it
doesn't change behavior. For example, consider the code:
    foo
        | bar |
        bar := baz someMessage.
        bar add: 1.
        ^bar
Now, if we inline the assignment, we really need to know what #someMessage
is doing. If it is returning a simple accessor, then we won't have any
problems. However, if it is returning a new collection, then we will have
problems. The RB doesn't perform any of the side effect checks that are
necessary to prove that it is a refactoring. Instead we let the user make
the decision on whether the refactoring is safe.

Anyway, to get back to your comment, you can fix the refactoring by changing
the InlineTemporaryRefactoring>>replaceAssignment. Change the
"assignmentNode parent isSequence" to be "assignmentNode isUsed not" (of
course you can eliminate the #not by reversing the ifTrue:ifFalse: cases).


John Brant

"John Brant" <[hidden email]> wrote in message
news:eAZ_8.1655$uh7.359@sccrnsc03...
But isn't that in any case equivalent to the following, with #yourself added
because of the semantics of #add:?

  foo
    ^(baz someMessage add: 1) yourself

But I guess that the RB would need to apply far more compiler construction
tricks than it currently does. :-)

Or would it not?

I don't know much about the internal mechanics of the RB.  A simple
replacement of the binding would result in

  foo
    ((baz someMessage) add: 1) yourself.
    ^(baz someMessage)

Which is bogus, of course.  But maybe we could state a rewriting rule that
says "include any following expression into the current expression if
possible", which would have this result:

  foo
    ^((baz someMessage) add: 1) yourself

Being correct.  (I know that this example is a bit contrived(?)...)

Or maybe introduce an intermediate temporary during refactoring:

  foo
    | tmp |
    tmp := ((baz someMessage) add: 1) yourself.
    ^tmp

Which could subsequently be removed.
It's all hypothetical, so I'll stop here... :-)

Cheers,
Andreas

(John, apologies for accidentally resending the original post by email)

How embarrasing.  I meant to use the cascade, sorry

>   foo
>     ^(baz someMessage add: 1) yourself

I really did mean to write:

  foo
    ^baz someMessage add: 1; yourself

and of course:

  foo
    | tmp |
    tmp := (baz someMessage) add: 1; yourself.
    ^tmp

...and so on.

"John Brant" <[hidden email]> wrote in message
news:eAZ_8.1655$uh7.359@sccrnsc03...
But isn't that in any case equivalent to the following, with #yourself added
because of the semantics of #add:?

  foo
    ^baz someMessage add: 1; yourself

But I guess that the RB would need to apply far more compiler construction
tricks than it currently does. :-)

Or would it not?

I don't know much about the internal mechanics of the RB.  A simple
replacement of the binding would result in

  foo
    (baz someMessage) add: 1; yourself.
    ^(baz someMessage)

Which is bogus, of course.  But maybe we could state a rewriting rule that
says "include any following expression into the current expression if
possible", which would have this result:

  foo
    ^baz someMessage add: 1; yourself

Being correct.  (I know that this example is a bit contrived(?)...)

Or maybe introduce an intermediate temporary during refactoring:

  foo
    | tmp |
    tmp := baz someMessage add: 1; yourself.
    ^tmp

Which could subsequently be removed.  It's all hypothetical, so I'll stop
here... :-)

I originally posted my reply through totallyobjects, but it seems that the
message didn't get through.  I then accidentally mailed John directly --
sorry John.  Even more embarrassing, I made a stupid mistake in the code
(forgetting the cascade...).  If you ever want to know how to make a fool of
yourself on usenet, this is one way to do it. :-)  Oh and please ignore any
of those other posts if they ever arrive...

Embarrassed,
Andreas

"Andreas Timmermann" <root@127.0.0.1> wrote in message
news:3d3db3e9@tobjects.... #someMessage
> > is doing. If it is returning a simple accessor, then we won't have any
> > problems. However, if it is returning a new collection, then we will
have
> > problems.
>
> But isn't that in any case equivalent to the following, with #yourself
added
> because of the semantics of #add:?
>
>   foo
>     ^(baz someMessage add: 1) yourself
>
> But I guess that the RB would need to apply far more compiler construction
> tricks than it currently does. :-)
>
> Or would it not?

No, it is not necessarily equal to "^baz someMessage add: 1; yourself". It
would be equal if #yourself is defined to always return self and is defined
for all objects. There are many proxy like objects that do not implement
#yourself so you need to make sure the object couldn't be one of those.

If you can verify that #yourself returns self, it still might not be the
"correct" thing to do. The original person who wrote the code could have
used a cascaded message without any temporaries so he must have had some
reason for not using a cascaded message (maybe he doesn't like cascaded
messages). Now, if we are to convert the code without cascades, to have a
cascade, then we probably aren't doing what the programmer wanted.

Anyway, if we decided that the programmer really wanted cascades, this only
addresses a small part of the overall problems with inlining temporaries.
For example, suppose we modify the example slightly:
    foo
        | bar |
        bar := baz someMessage.
        1 printOn: bar.
        ^bar
In this case, "bar" is most likely a stream, and we cannot convert it to be
a cascaded message. If #someMessage is creating a new stream, then we are in
trouble since we will be return a new stream instead of a stream with "1" on
it. However, if stream is returning the same object for each call, then we
have no trouble.

Here's another example:
    foo
        | bar |
        bar := baz.
        ...some code...
        ^bar
If we are to inline "bar", we need to prove that "...some code..." never
modifies the instance variable baz. Most of the time when someone writes
code like this, "baz" is being modified, but they might have just performed
some other transformation where baz is no longer being modified and they
wish to remove the unnecessary assignment.

I had a similar discussion with Kent Beck before inline temporary was
implemented. He wanted inline temporary, but I was arguing that in
Smalltalk, it is almost impossible to implement inline temporary as a useful
refactoring (that does not change behavior). If I implemented it as a
refactoring, then most likely most of the time when someone tried to use it,
it would fail -- not being able to prove that it could be done safely. Since
people stop using things that fail frequently, there was no need for us to
implement it. His response was that whether or not the tool supported it, he
had to perform an inline temporary transformation and that the tool could
perform this transformation better than he could manually (both in terms of
speed and correctness -- e.g., it would not mess up comments/messages with
the same name). Anyway, his argument made sense so we implemented inline
temporary as a transformation.


John Brant

John Brant wrote:
I can't see that that makes any difference.  The way to inline the
above is to replace it by a cascade, i.e.

        (baz someMessage add: 1; yourself)

This is exactly equivalent apart from the sending of #yourself.
I think its an acceptable caveat that the result of baz someMessage
should not redefine #yourself for the refactoring to preserve
semantics.  But the cascade does actually create an anonymous
temporary variable on the stack to hold the result of baz someMessage,
so provided that the implementation of #yourself is ^self, then
(baz someMessage add: 1; yourself) has exactly equivalent semantics
to a send of foo, except for the activation of foo.

--
_______________,,,^..^,,,____________________________
Eliot Miranda              Smalltalk - Scene not herd