parser is broken by literal array with square bracket about tonel

[dalehenrich]

just a wild thought but what about making leading white space for method source a requirement and the enclosing brackets are on lines by themselves:

[
  "What about this valid literal Array guys" ^#( [ ) 
]

... and the parser chokes with a meaningful error if there is a non [] character in method body in position 1...

It's not quite ideal, but it does mean that the extraction of method is fast and humans can learn to follow the rule ... only folks who don't believe in whitespace will be unhappy:)

[estebanlm]

heh... I do not believe in whitespace ;)
I have already a fix for both cases, I'm refactoring to get it right (into a new class TonelSourceScanner)

[dalehenrich]

... i don't like it either:) but compromises have to be made somewhere ... BTW, I also prefer the parsing approach, because the source needs to be unadulterated ... martin mentioned that you are also eliminating symbols from the meta data ... have you made those changes as well ... I already went through and added asSymbol is the few places where GemStone uses strings, so I'll have to clean that up, too:)

[estebanlm]

yeah, I have an already working version: just ugly and needs refactor before committing.
about the categories: I told Martin I'm ok with that (basically, it does not changes anything to me so if you guys prefers it, I'm fine with it). But I still didn't have the time to implement it :)
(I need to add an issue, btw)

[nicolas-cellier-aka-nice]

this issue should be reopened because the fix is too simplistic...
Imagine that I have a method like this:

closingChars
    ^#( $) ] } )

then current fix will detect the end of literal array just after $), which is not OK.
Also I can put comment or string inside litral arrays...

closingExpressions
    ^#( "note that we need to prefix ) with $, but this is not necessary for ] and }"
          ')]}' $) ] }
    )

And code like above could really exist!

That's what I don't like about the Tonel format, it must be too much aware of Smalltalk syntax.
If I want to implement some extensions like Dolphin ##( ) then there will be yet another syntax in the literal array...

[estebanlm]

yes, it could.
At a point, the real implementable solution for us is to use RBParser to solve the parsing and just forget about it... but then it will not be portable anymore :)

[nicolas-cellier-aka-nice]

Yes, RBParser would likely kill cross-dialect portability.
I suggest that we parse the literal array more thoroughly with same kind of hacks as upper level

• recognize and skip comments
• recognize and skip strings
• recognize and skip chars

maybe it'll be enough (until someone find an even uglier piece of syntactically valid Smalltalk)

[nicolas-cellier-aka-nice]

Oh, but I see that it is already doing this... Only the Character literal test is missing.

So maybe just a $ protection should do it

scanForLiteralArrayIfMatch: aBlock
| literalCount |

self isStartingLiteralArray ifFalse: [ ^ self ]. 
literalCount := 1.
self readUntil: [  
	self scanForStringIfMatch: [].
	self scanForCommentIfMatch: [].
	(char = $( and: [prevChar ~= $$]) ifTrue: [ literalCount := literalCount + 1 ].
	(char = $) and: [prevChar ~= $$]) ifTrue: [ literalCount := literalCount - 1 ].
	literalCount = 0 ].
aBlock value

[nicolas-cellier-aka-nice]

Unfortunately, the (prevChar ~= $$) test is also too simplistic.
Inside a literal array #( ... ), nothing prevents a $$ to be immediately followed by another character...

The most simple counter example is

literalCharacterPrefix
    ^#($$)

More complex ones are:

^#( $$'please use space before this string)('$$"please use space before this comment or avoid putting a ] in it"$$(this really is a literal array with a ] in it))

The problem is not only inside literal, it is also in blocks:

maybeReturnADollar
    self testSomething ifTrue: [^$$]

Which just mean that literal char ($) must be properly parsed, like literal array, string and comments already are...

[nicolas-cellier-aka-nice]

Solution is to properly parse literal char, then simplify (remove) prevChar tests like this:

isStartingLiteralCharacter
"A literal Character start with a dollar sign in Smalltalk syntax"  
^ char = $$

isStartingComment
"Comment start with a double quote in Smalltalk syntax"
^ char = $"

isStartingString
"String start with single quote in Smalltalk syntax"
^ char = $'

scanForLiteralArrayIfMatch: aBlock
| literalCount |
self isStartingLiteralArray ifFalse: [ ^ self ]. 
literalCount := 1.
self readUntil: [
	self scanForLiteralCharacterIfMatch: [].
	self scanForStringIfMatch: [].
	self scanForCommentIfMatch: [].
	char = $( ifTrue: [ literalCount := literalCount + 1 ].
	char = $) ifTrue: [ literalCount := literalCount - 1 ].
	literalCount = 0 ].
aBlock value

scanForLiteralCharacterIfMatch: aBlock
self isStartingLiteralCharacter ifFalse: [ ^ self ]. 
self readNext.
char := prevChar := Character null.
^aBlock value

scanNextChunk
self readNext.
self scanForLiteralCharacterIfMatch: [].
self scanForBlockIfMatch: [ ^ self ].
self scanForCommentIfMatch: [ ^ self ].
self scanForStringIfMatch: [ ^ self ].
self scanForLiteralArrayIfMatch: [ ^ self ]

The parser code is a bit simpler and more robust...
It still knows too much about Smalltalk syntax, but that we cannot fix, it is flawed by design.