Automatic source code formatting bug.

Hi.

  I believe I may have run into a bug with Dolphin 5.1.3. automatic
source code formatting. And, if it is a bug - then it's dangerous
because it changes the meaning of a method call but still generates
valid syntax.

Here's an example:

---Original code:---

    (ClassA new)
        method1;
        method2 printString;
        yourself

---Reformatted code:---

   ((ClassA new)
        method1;
        method2)
        printString;
        yourself

  Is this a bug? Is the first expression somehow ambiguous? Am
I missing something here?

  Which ever way it is my app isn't working after being ported to
Dolphin 5.1.3. because this bug bit it! :-(( And god only knows
in how many places it got bitten :-(((

  Thanks in advance,
    Jurko

Jurko,

>     (ClassA new)
>         method1;
>         method2 printString;
>         yourself

>   Is this a bug? Is the first expression somehow ambiguous? Am
> I missing something here?

The reformatting looks correct to me and I don't think it is ambiguous
at all (a bit confusing maybe).

A cascade comprises of a series of statements in which all messages are
sent to the receiver of the first message in the cascade (ie the first
message send that is followed by a semi colon) and whose answer is the
answer from the last message send (i.e. the first message send that is
not followed by a semi colon)

The above therefore has two cascades

ClassA new
    method1;
    method2

and

(the answer returned by method2)
    printString;
    yourself

which is what the reformatting provided.

What were you expecting the original code to answer? Should #yourself
answer the result of sending printString to the answer returned by
#method2 or the answer from ClassA new.  If it's the former then you
want

((ClassA new)
    method1;
    method2) printString

but if it's the latter (and assuming #printString doesn't have any side
effects) you want

(ClassA new)
    method1;
    method2;
    yourself

To exactly replicate what I think you were trying to achieve (assuming
#printString does something useful rather than just returning a value) I
think you will need to use a variable.

a := ClassA new.
a method1.
a method2 printString.
a

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

Hi Ian.

  Thanks for answering.

> The reformatting looks correct to me and I don't think it is ambiguous
> at all (a bit confusing maybe).

  I don't think we understood each other the first time... I still feel
there's
something fishy here... look at this example:

1. Create a class named Test.
2. Add method 'Test>>doNothing' that does just what it's name says.
3. Add method 'Test>>return5' that does '^5'.

  Now evaluate the following in a Dolphin workspace:

    (Test new)
        doNothing;
        return5 printString;
        yourself.

  You should get 'a Test' as a result.

  Now try to evaluate the following:

    ((Test new)
        doNothing;
        return5)
        printString;
        yourself.

  You should get '5' as a result.

  So obviously - those two code segments don't do the same thing, but if you
type in
the first one in some methods body and have RB reformat it, it transforms it
into the
second one. As I see it - eather RB reformats the first expression wrong or
Dolphin
evaluates the first expression wrong.

  Best regards,
    Jurko

Jurko,

>   I don't think we understood each other the first time... I still
> feel there's
> something fishy here... look at this example:

OK, I'm with you now.  You are pointing that an method that just
accepted without reformatting answers a different value to one that is
reformatted but both versions compile and evaluate without error.  I
thought you were just saying that the reformatter was wrong.

>   So obviously - those two code segments don't do the same thing, but
> if you type in
> the first one in some methods body and have RB reformat it, it
> transforms it into the
> second one. As I see it - eather RB reformats the first expression
> wrong or Dolphin
> evaluates the first expression wrong.

>From my reading of the ANSI spec (along with "Smalltalk: Best practice
patterns" and the syntax diagrams in the Blue Book) I would say that
Dolphin is parsing the expression incorrectly and that the reformatter
gets it right.

I'm not sure about the validity of something else I noticed as well

aMethod
    ^'abcde' ; first ; last

Just use "Accept" and it compiles and answers the expected (?) value.
Using "Reformat/Accept" fails with compilation error.

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

Jurko Gospodnetiæ wrote:

>   I believe I may have run into a bug with Dolphin 5.1.3. automatic
> source code formatting. And, if it is a bug - then it's dangerous
> because it changes the meaning of a method call but still generates
> valid syntax.

I asked about this a year or so back when the deformatter first came out and
Blair said that it should be considered a bug in Dolphin's parser.

A bit of background for anyone who is interested (you can read more by
searching c.l.s on Google -- try looking for my name and "syntax"):  The ANSI
Smalltalk spec defines Smalltalk to have a syntax that allows more complicated
cascades than common practise (and the Blue Book) did.  It also appears to
define a different semantics for them.  It has since been stated that this was
just an error in the standard.  Which, btw, I find difficult to believe -- it's
not the kind of error that would happen just "by accident".  Anyway, and be
that as it may, it is *functionally* an error in the standard since nobody has
any interest in changing their Smalltalk implementation to meet that part of
the spec.

Dolphin came out after the spec was at a fairly advanced stage and
(paraphrasing my memory of comments that Andy and Blair have made over the
years) they attempted to follow the spec in this matter -- a decision that they
have since come to see as a mistake.  So, although the Dolphin parser has
always accepted the wider ANSI syntax, that's really just a historical glitch,
and shouldn't be relied on.  There was once some code in the image that used
the extended syntax, but that has long since gone away.  And, as you have
noted, the RB parser/formatter doesn't handle it in the same way as the Dolphin
parser.

BTW, I personally think that the syntax and semantics that ANSI came up with
(even if it was by accident) make more sense than the traditional Blue book
spec (another reason why I *don't* believe it was an accident ;-).  However, it
wasn't the committee's role to create a new language (even if the C++ people
did just that, and quite successfully) and I think that the various implementer
are probably right to ignore that part of its wording.

    -- chris

Hi Chris.

  Thanks for answering.

> I asked about this a year or so back when the deformatter first
> came out and Blair said that it should be considered a bug in
> Dolphin's parser.

  If this is so, then does anyone know if it'll get fixed? And if so, when?

  As it is now - I'm in a bit of a mess... I've got a lot of legacy code
that needs to be ported to the new version of Dolphin, but in it I've
found code that depends on the way Dolphin' parser handles this.
On the other hand, if I use the reformatter, program's semantics
change without me knowing about it :-(.

--- <whining> ---
  Not to mention that this legacy code has no unit tests or any other
way of detecting invalid behavior except manually testing the
application... Right now, I might have fixed it all... but then again I
can't be sure - :-) might just bite me in the 'behind' after a new
version of the application gets shipped... :-((

  God, I hate being a maintenance programmer! :-))))
--- </whining> ---

  Also are there any other quirks to using the reformatter? Because,
so far I've found it an extremely useful tool. It aggressively changes the
code structure, but so far, every time it made the code look bad -
I needed to refactor and clean up the code anyway. :-))).

  Best regards,
    Jurko

Jurko,

>   As it is now - I'm in a bit of a mess... I've got a lot of legacy code
> that needs to be ported to the new version of Dolphin, but in it I've
> found code that depends on the way Dolphin' parser handles this.

Difficult.

One approach would be to use the "classic" method.  Write one or more unit
tests *before* letting the RB do any refactoring (or doing any by hand).
According to the pundits that's what you "should" be doing anyway, and the
possibility of semantic changes only adds more motivation.

OTOH, that's not what *I* would do ;-)

What *I* would do is just not use the RB -- I find too destructive of important
accesory information in the code (such as layout), and I don't *ever* use it.

But, assuming that you do want to use the RB's refactorings, but don't want to
write unit tests for everything it does (and after all, there's not a lot of
point of an automated tool that needs to be backed up by hard-written tests),
then you need a way to identify the "problem" code sequences automatically.

You might be able to do it with the RB's parser itself.  Code that it can't
parse is obviously in the problem category, so you can identify that easily.
Code that it can parse, but parses differently, can probably be identified from
the parse tree, or maybe using an RBSearchRule or ParseTreeSearcher. I don't
know the RB facilities well enough* to know how to do it myself, but it seems
as if it *should* be possible.  Perhaps some RB expert is reading this and can
suggest something.

Failing that, somewhere I have a parser for Dolphin Smalltalk which recognises
the extended cascade syntax.  I haven't touched it for a couple of years, but
it probably still works and I could dig it out if you need it.


> Because,
> so far I've found [the RB] an extremely useful tool. It aggressively changes
the
> code structure, but so far, every time it made the code look bad -
> I needed to refactor and clean up the code anyway. :-))).

You may not have noticed that you can change the default layout it uses.  The
result has still had all the accessory information that I mentioned earlier
stripped out, but it can at least be a bit prettier.  Go to the
tools=>options=>inspect menu of the "System Folder" window and then the
'formatterClass' item under the 'Development System' sub-tree.  Ensure that the
formatter class is 'RBConfigurableFormatter' and then you should have lots of
options to twiddle.

    -- chris

[*] Or at all, to be honest.

Hi Chris.

> You might be able to do it with the RB's parser itself.
> ...

  I've already been thinking about trying that. Will let you
know how it works out if I decide to go that route.

> Failing that, somewhere I have a parser for Dolphin Smalltalk
> which recognises the extended cascade syntax.  I haven't
> touched it for a couple of years, but it probably still works
> and I could dig it out if you need it.

  Thanks, but don't want to bother you for now. As above, I'll let
you know if I decide to try it. Thanks again for mentioning it.

> Ensure that the formatter class is 'RBConfigurableFormatter'
> and then you should have lots of options to twiddle.

  I already did that before. :-) But I fear this is not enough to
solve all my problems. :-)) This legacy code contains many
'C-like' methods spanning multiple pages, doing different
things at once with hacks and quick-fixes spread all through
them. As it is now - they aren't readable in the first place and
I fear that there doesn't exist a uniform set of reformatting
rules that could make them readable without manual
refactoring... :-)))

  Thanks again for your suggestions!

  Best regards,
    Jurko

Jurko,

>   As it is now - I'm in a bit of a mess... I've got a lot of legacy
> code that needs to be ported to the new version of Dolphin, but in it
> I've found code that depends on the way Dolphin' parser handles this.
> On the other hand, if I use the reformatter, program's semantics
> change without me knowing about it :-(.

For a quick test you could scan all the methods to check that the byte
code generated by the normal and formatted source is the same.

Class allMethodsDo: [:each |
bytes1 := (Compiler compile: each getSource in: each methodClass)
byteCodes.
bytes2 := (Compiler compile: (SmalltalkParser parseMethod: each
getSource in: each methodClass) formattedCode in: each methodClass)
byteCodes.
bytes1 ~= bytes2 ifTrue: [self halt]]

This will obviously only work if you still have the original (i.e.never
been reformatted) source in the image.

--
Ian

Use the Reply-To address to contact me.
Mail sent to the From address is ignored.

Hi Ian.

> For a quick test you could scan all the methods to check that the byte
> code generated by the normal and formatted source is the same.

  Thanks!!! I'll try that at once!

> This will obviously only work if you still have the original (i.e.never
> been reformatted) source in the image.

  Hehe, not in the image, but that's why I keep everything in reloadable
packages and backup often. :-)))

  Thanks again!

  Best regards,
    Jurko

Hi Ian!

> > For a quick test you could scan all the methods to check that the byte
> > code generated by the normal and formatted source is the same.

  Tried it, and it worked like a charm! :-)) It turned out that I managed to
manually get all the bugs before, but it gave me a peace of mind and also
shook out another type of 'bug'. :-)

  It seems that the old Dolphin compiler allowed for cascades to have ';'
as their last character while the new one doesn't... not a big thing, but
with your code finding all occurances was a breeze! :-))

  Thank you!!!

  Best regards,
    Jurko

Hello all,

> What *I* would do is just not use the RB -- I find too destructive of
important
> accesory information in the code (such as layout), and I don't *ever* use
it.

I largely agree with Chris.  However, the RB does several things that do not
require rewriting code, and as such do not scramble comments etc.  I am also
quite willing to use the RB on new code; otherwise, I must agree that
"destructive" is a good word, at least with the current parsing and
formatting.

Have a good one,

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]

Jurko,

Whilst I respect the right of Chris, Bill (and maybe others) to air
their views I really must suggest that you come to your own conclusions
about the viability/usefulness of the Refactoring Browser to the way
you work with Dolphin.

Personally, I find the RB *absolutely* indispensible and I would hate
to think that you should be disuaded from using it because of some
nebulous fear factors that, although they may intefere in the way Chris
and Bill work, may well be of little consequence to you.

Ok, the code rewriter does "destroy" code layout and there are some
other problems (as you have noticed) with disparity between the
underlying Dolphin compiler and the RB parser. But remember, using the
automatic formatting frees you from all thought of how to lay out your
code. Interestingly, I didn't like the way the RB formatter laid out
code either at first but soon came to appreciate the fact that I no
longer had to worry about doing it myself. Indeed, I now just type code
all on one line at type Ctrl+Shift+S to compile and format it all in
one go.

Given the fact that, if you accept the automatic formatting, you then
have access to a remarkable code maintenance tool (the RB) I think it
is worthwhile trying to see if your working style is malleable enough
to accept some new practises.

Best regards,

 
Andy Bower
Dolphin Support
www.object-arts.com

Hi Andy.

> Personally, I find the RB *absolutely* indispensible and I would
> hate to think that you should be disuaded from using it because of
> some nebulous fear factors that, although they may intefere in the
> way Chris and Bill work, may well be of little consequence to you.

  So far, I'm using it and am finding it to be a most useful tool :-)

> Ok, the code rewriter does "destroy" code layout and there are
> some other problems (as you have noticed) with disparity between
> the underlying Dolphin compiler and the RB parser.

  The layout - I don't mind much. As it is now - it promotes the
Smalltalk way of programming (short methods, many small objects -
- the way I prefer anyway) without explicitly forbidding any other
style, so legacy code can still be (re)used.

  However... I do mind that there is a disparity between the Dolphin
compiler and the RB parser. That is dangerous - especially when
porting legacy code, which may depend on the way that Dolphin
compiler works.

  Will that get fixed? If it is, in which Dolphin version can we expect
to see it? While researching this problem I read some old posts on
this subject where I saw Object Arts (don't remember who exactly)
saying that Dolphin would eventually use the RB parser, but is that
idea in any active development stage, or is it still just an idea? Does
D6 still use it's own proprietary compiler?

  Best regards,
    Jurko

Jurko

It is on the list of things to do but it is unlikely to be addressed in
D6.

Best regards,

Andy Bower
Dolphin Support
www.object-arts.com

Andy Bower wrote:

> Whilst I respect the right of Chris, Bill (and maybe others) to air
> their views I really must suggest that you come to your own conclusions
> about the viability/usefulness of the Refactoring Browser to the way
> you work with Dolphin.

Actually, given the mess that Jurko has inherited, it sounds as if the RB would
be my tool of choice too.  After all there isn't going to *be* any sensible
out-of-band infomation in what sounds like a classic "ball of mud".

At least, that'd be the case if I didn't have the option of a complete
re-write.

    -- chris

"Jurko Gospodnetiæ" <[hidden email]> wrote in message
news:c21faq$6d8$[hidden email]...
> ...
>   However... I do mind that there is a disparity between the Dolphin
> compiler and the RB parser. That is dangerous - especially when
> porting legacy code, which may depend on the way that Dolphin
> compiler works.
>

In this case your best bet is to run a "smoke test" on the reformatter which
will highlight any areas where Dolphin's extended cascade syntax has been
used. This can be done by comparing parse trees, i.e. enumerate all relevant
methods, compile the existing source to build a parse tree, and then
reformat and builds a parse tree from the reformatted result. Then compare
the two and if they differ then the reformatting has changed the meaning of
the code. In fact the RB tests include such a test, below.

Regards

Blair


-----------------------
FormatterTest>>testSmokeTest
 Refactoring withAllSubclasses do:
   [:class |
   class selectors do:
     [:sel |
     | tree |
     tree := class parseTreeFor: sel.
     self assert: tree = (RBParser parseMethod: (RBFormatter1 format:
tree))]]

well...

i have my opinions about how code should be formatted, but it is
more important to me that all code on a project be formatted the
same way rather than it be formatted -my- way.

people who get wrapped around the axle about code formatting
need to refocus their efforts.

my $0.02 worth


"Chris Uppal" <[hidden email]> wrote in message
news:40446b52$0$1164$[hidden email]...
> Andy Bower wrote:
>
> > Whilst I respect the right of Chris, Bill (and maybe others) to air
> > their views I really must suggest that you come to your own conclusions
> > about the viability/usefulness of the Refactoring Browser to the way
> > you work with Dolphin.
>
> Actually, given the mess that Jurko has inherited, it sounds as if the RB
would
> be my tool of choice too.  After all there isn't going to *be* any
sensible
> out-of-band infomation in what sounds like a classic "ball of mud".
>
> At least, that'd be the case if I didn't have the option of a complete
> re-write.
>
>     -- chris
>
>

Donald MacQueen wrote:

> people who get wrapped around the axle about code formatting
> need to refocus their efforts.

Just to be clear.  My objection to the RB's reformatting is not, and never has
been, that it formats the code "wrongly" -- that's not the issue at all.

    -- chris

> Just to be clear.  My objection to the RB's reformatting is not, and never
has
> been, that it formats the code "wrongly" -- that's not the issue at all.

Likewise.  It moves comments to the point that efforts to communicate with
myself and others are thwarted at best, and can become flatly misleading.
Unit tests are wonderful, but they are not magic, and do not guarantee a
quality product.

Bill

--
Wilhelm K. Schwab, Ph.D.
[hidden email]