Ticket #211 (closed enhancement: fixed)

Opened 8 years ago

Last modified 6 years ago

Add implicit line continuation for lines ending in an operator.

Reported by: nevdelap Owned by: Chuck
Priority: minor Milestone:
Component: Cobra Compiler Version: 0.8.0
Keywords: Cc:


I searched for a ticket along these lines and didn't find one so I'm adding this just so it doesn't fall through the cracks. Comes out of...

Chuck, "I was thinking that if the line ends on an operator then it's pretty obvious that it continues."

Example: (Alignments will be correct when pasted into your editor.)

class Program

	def main
		print 1 + 2		# ok
		print (1 +		# ok
		print 1 +		# This ticket: Expecting an expression.
		print (1 +		# Ticket 210: Cannot mix tabs and spaces in indentation.
		print 1 +		# Both tickets.
		a = 1
		b = 2
		assert (a == 1 and	# ok
			b == 2)
		assert a == 1 and	# Both tickets.
			   b == 2


implicit-line-cont.patch Download (5.3 KB) - added by hopscc 8 years ago.
implicit-line-cont-preparser.patch Download (5.8 KB) - added by hopscc 8 years ago.
implicit-line-cont-expr.patch Download (4.7 KB) - added by hopscc 8 years ago.
implicit-line-cont-expr2.patch Download (6.4 KB) - added by hopscc 8 years ago.

Change History

Changed 8 years ago by Chuck

I agree.

Changed 8 years ago by hopscc

  • owner set to hopscc
  • status changed from new to assigned

Changed 8 years ago by hopscc

Changed 8 years ago by hopscc

  • owner changed from hopscc to Chuck

Patch, new testfile and relnote update.

I had to add a bit of a hack to the CobraTokeniser to avoid/workaround false positives
detecting/handling line continuation on trailing generic and stream type declarations (trailing '*' and '>') that I'm not entirely happy with but it seems to work satisfactorily and passes all the tests now which wasnt the case with the first two attempts made.
Its possible that there may be some cases that due to this line continuation with trailing * or > token isnt correctly recognised and handled (though I've not seen any to date).

The operator tokens for recognising this implicit line continuation is a subset of the BinaryOperator Tokens in CobraParser.cobra - the list is reordered and stored in CobraTokenizer.cobra (implicitContinuationOps).

Changed 8 years ago by Charles

  • owner changed from Chuck to hopscc

I agree with the ticket, but have concerns about the patch. My impression is that this was implemented in the wrong place. Can this be implemented in the parser? The issue of continuing a line on a binary operator is an issue of grammar where the binary ops are already specified. Maintaining _implicitContinuationOps seems dubious and a duplication of specs in the parser. Maintaining _keywordsSuppressingILCont and the special check for * and > is also undesirable.

I haven't tried a parser-based implementation, so maybe there is some issue there I am not aware of.

Changed 8 years ago by hopscc

  • owner changed from hopscc to Chuck

Short answer is yes.

Long answer
My first attempt implemented it in the parser. (Preparser).
It was somewhat simpler code but a lot easier to get screwed up with variable input as it turned out more susceptible to variances from the format of the continued and following lines since it had to look forward (or keep a hanging count) find and remove spurious dedents for any indents introduced - I found it was not difficult to provide source code that caused removal too many or not enough dedents

(the processing for this for explicit continuation token is already done inline in the tokeniser stream which is why I switched over to using that).

I suppose other approaches (e.g on parsing each binary op ) are possible.

_implicitContinuationOps needs pulling out separately since not all the binary ops
are (or should be) candidates for continuation. Its also reordered as a lookup optimisation - arguably this info could be added to the binary ops table and passed around OTOH its not required for all ops.

I disagree about the duplication of specs wrt this (and any maintenance burden), They are being used for different purposes - In the tokeniser they're purely regarded/used as (implicit continuation) tokens, theres no requirement that they be only binary ops (though thats the capability being supported)

I know and agree that _keywordsSuppressingILCont is undesirable - that in fact is what the 'bit of a hack' statement above refers to.
Unfortunately thats purely cos the use of some tokens is overloaded at EOL - Generic and Streams Types vs GT and multiplication expr (with continuation) trailing '>' and '*'.

I dont have the original parser code changes anymore but given enough motivation I may be able to recreate them for an alternate patch. For the moment thats lacking.

Changed 8 years ago by Charles

Which binary ops should not be used for line continuation and why?

Changed 8 years ago by hopscc

Any that dont make sense to be broken across a line
DOT for one... TO/ TOQ

For a first pass see items not in _implicitContinuationOps

ops that while binary are clearer being presented associated with their left and right operands on the same line
Ops that for clarity should have short obvious operands

Changed 8 years ago by hopscc

Changed 8 years ago by hopscc

Heres something along the lines of the original code I did in the parser (preparser/Tokenizer) - somewhat tweaked.
It doesnt support mixing Tabs and whitespace in continued lines as the initial patch did.

Changed 8 years ago by hopscc

Changed 8 years ago by hopscc

Heres an implementation based on the expression clause in the parser.

It doesnt have the token orientation and false positive avoidance of the other two implementations but it requires some measure of lookahead and modification of the token stream while in the internals of expression parsing to cleanup
removed indentation.

Changed 8 years ago by hopscc

I found some problems with the implementation based in the expression clause (previous patch) and the tests were missing some obvious cases.

Heres a reimplementation still using the expression clause and augmenting the expression parsing for if/[post] while.
Changed to use _spaceAgnostic processing so avoids specific token stream lookahead/cleanup.
Includes code from patch for ticket:116 ( but not its tests)

Changed 8 years ago by hopscc

Changed 8 years ago by Charles

I'll have a look.

Changed 6 years ago by Charles


Changed 6 years ago by Charles

  • status changed from assigned to closed
  • resolution set to fixed

Applied in changeset:2783 which also fixes ticket:116

Note: See TracTickets for help on using tickets.