Compile error with both isnames and assignment on class var

[hopscc]

lines of this form cause a compile error

var name = 'xxx' is public # or any isname

Heres a sample program

class VarIsNameAssign
	var myVal0 is shared
	var myVal1	 = 'works'
	var myValBroke = "now not broke" is shared # 
	
	def main is shared
		assert not .myVal0 
		v = VarIsNameAssign()
		assert v.myVal1 == 'works'		
		a = v
		b = VarIsNameAssign()
		assert a is v
		assert .myValBroke == 'now not broke'	# compile error here when broken
# error: Expecting an expression. "shared" is a reserved keyword that is not expected here.		
		assert a is v
		assert a is not b

Due to use of full expr when parsing the assignment - it sucks up the 'is isnameValue' thinking its an 'isRef' check.

The compiler has code to support this (assignment and isnames) but theres no test case for it.

[hopscc]

ANd heres the patch for it
Code mods + new testfile + rel notes update

[Chuck]

"is" really is a binary boolean operator. I suppose it's unlikely to be used in initialization code, but it looks weird to me. Currently, Cobra let's you put it on a separate line which makes it more obvious what's going on:

class Foo

    # works now
    var y = 5
        is public

    # another idea
    var y = 5, is public

    # also this will eventually work:
    public
        var y = -1
        var x = -1

Also, public vars are fairly rare.

I want to think about this some more. Feel free to add comments.

[hopscc]

The problem is the use of 'is' as a binop in expressions (fine) and the use
in var decls for specifying/leadin to isNames attribs(also fine) - but given
var decl syntax mandates assignment before isnames setting theres a possible conflict in the meaning (resolved currently and wrongly now by taking the 'is' as part of the expression).

Could remove ambiguity by changing syntax to specifying isnames before assignment
i.e var decl form becomes

var <name> [as type] [is isnamelist] [ = expression]

though thats perhaps less
visually attractive (??) and possible problems with parsing rest of clauses (docstring, attribs, ??) ? - need to look at that.

Desire/Use? for 'is' as part of expression in a var decl is only likely if the var decl is for a bool

var otherVarIsX as bool = anothervar is X # is private

I thought that use probability was low enough to suppress it entirely for var decls
though its not totally unlikely...

If the meaning can be disambiguated by breaking the line before the isnames setting
and its thought the is-in-assignment-expr is needed (in var decls) then perhaps its enough to trap the error caused by use of is <keyword> treated as an expression (for var decls) and change the error to mention the ambiguity and
mention the availability of the broken line (before 'is') syntax form.

I dont like the , form at all - smacks of bandaid syntax hackery .

  var y = 5, is public, shared
           ^ Huh?
                      ^ OK expected these are a poss list of isnames

I'm not fond of the broken line form either(nl+indent sometimes allowed and significant at special places (undoc) in decls) but can probably live with it
(needs doccing).

[Chuck]

The approach I want to take is:

• make public, private, etc. keywords
• give an informative error for "var y = 5 is public"
• add support for saying "public" and indenting like one can do with "shared"

[hopscc]

Well thats fine as far as it goes but it still doesn't address the problem which is to allow the seemingly valid simple single line var form ( dcl var, with assignment and access modifier )....

the (single) keyword indenting clause described may be fine for multiple single isnames but wont cleanly address combinations ( shared, {public,protected,private} ... and it gets worse if const, readonly and volatile are ever supported...)

Thinking about it some more I think the patch I supplied is wrong
(disallowing 'is' completely in var assignment initialisation)
A better solution is to either change the (less common) use of 'is' in an expression for testing equivalence/aliasing

var avarEq = avar is anotherVar
var ynil = y is nil
...
  if z is nil, ...

to something else - I'd suggest 'issame'

var avarEq = avar issame anotherVar
var ynil  = y issame nil
if z issame nil, ....

( thats probably non optimal due existing use/code(!))

OR
allow a synonym for the 'is' keyword in var decls (at least, perhaps elsewhere for all isnames for consistancy) - and modify the error message when using 'is' pointing to the more verbose keyword

here I'd suggest a longer form with an is suffix e.g. 'isdecl' or 'ismade'

var y = 5 ismade public
var z = 5 isdecl shared, public

[hopscc]

working on patch for change as described

[hopscc]

patch file for making 'isdecl' a synonym for 'is' (or vice versa) for declaring modifiers working around the ambiguity/impossibility of having a single line boxvar with both assignment and modifier specification

var x = 'defaultValue' isdecl shared, public

Test files to show original problem and highlight workaround

[Chuck]

I reviewed this today from top to bottom.

Recall that in Cobra visibility can be done with underscores:

    var foo   # is public
    var _foo  # is protected
    var __foo # is private

So there's one way to specify the visibility.

Additionally, "is <modifier>" can be put on the next line, the same as with types (class, struct, interface) and members (method, property, indexer). So there's another way, and it covers all modifiers.

Additionally, we'll add:

class X
    public, shared
        var x = 0

That would make for three ways.

Additionally, we'll give an informative error for "var y = 5 is public".

Overall, I didn't like the keyword synonym idea. But more importantly, the "public, shared" section idea is going to be implemented anyway, so might as well try that first.

[hopscc]

The problem is not (just visibility/modifiers/isnames) its specifying both that and assignment (and in a single line)

The most obvious syntax just doesnt work.

var mine = 99 is protected,shared # assignment and explicit modifier specified

Yes you can break it across lines ( if you realise thats supported - thats not obvious)
yes you can/will use modifier/visibility sections - a little baroque for a single
entry though and may get interesting for permutations.
Yes you can use the '_' syntax for some (the visibility) modifiers. Not ideal if you want to expose the visibility explicitly in code though.

Wouldnt it be better than rather than giving an informative message saying 'you cant do that that way, you have to do it this way' you just support it? (especially as to my mind its the most obvious expansion of the syntax;

var with assign, var with isnames,
var with isnames and assigned is just appending clauses together)

Is there any syntax that supports both assignment and isnames on a single line that
avoids a keyword synonym (beyond disallowing 'is' in assignment as the first patch)?
I couldnt come up with one but ...?

Is it yet possible to swap the order of the isname and assignment clauses (as mentioned above).

var xVar  as <type> is <modifiers> = <initValue> # change

That seems a little more reasonable cos the isnames binds to the variable not the
assignment value which is the impression from the current ordering

var xVar  as int = 99 is private, shared  # currently
# vs
var xVar as int is private, shared = 99

[hopscc]

Touch to raise into last 50

[hopscc]

This stupid fracking bug bit me again ... You dont need it very often but when you do the simplest obvious thing doesnt work.

When I finished what I was doing and stopped fuming I had another pass over this report and the patches to date - both of which I now dislike nearly as much as I dislike the current status quo
so I did what I suggested early in the piece and should have tried earlier on and changed the ordering of the isnames and initialExpr clauses (initially in the single line version).

This passed all the tests AND fixed the failing test above....

Somewhat encouraged and unhappy with the thought that the single line and multi line variants now used different clause ordering I redid the patch so both variants used the same (new) clause ordering - isnames clause before assign-initialExpr clause

Passes failure situation again (field decl with both isnames and initialExpr on 1 line)
but failed in (only) 3 tests all of which corresponded to the use of both isnames
and assign-initialExpr clauses using the erstwhile only-one-that-compiles old multiline variant

var id = <initVal>
    is shared  # in this case

(One of these 5x was in CobraCore).
a 1 line patch to my change supported that as a special case also...

all tests pass, failure situation passes
compiler self hosting rebuilds compiler, compiles itself and rebuilt compiler
runs on tests .... all work

SO

Heres a code patch that swaps the clauses, with changes to use the new form in tests and libs where it was using the old syntax even though these will still compile and a humongeous new test case to exercise all the variants of field decl, including the one that used to fail, and several that the parser supports (and supported) but havent been used in compiler or test code SFAICT.

You'd thing swapping clause order would cause a myriad of problems in existing code (I did) but the most common uses dont seem to use both these clauses at the same time so while the change sounds horrendous to existing code in 846/849 (99.646 %) plus of cases it actually makes no discernable difference...

[hopscc]

Also reported ticket:252

[Charles]

bump

[Charles]

changeset:3104

[Charles]

I'm reopening this ticket since the patch breaks the Cobra IDE add-in:

Project\CobraCompilerParameters.cobra(100,3): error: Expecting DEDENT, but got "has" instead.
Project\CobraProjectParameters.cobra(21,3): error: Expecting DEDENT, but got "has" instead.

    var _performanceQualityOption = PerfQualOption.Default
        has ItemProperty("PerformanceQualityOption", defaultValue = PerfQualOption.Default)

Reverting to 0.9.6 restores the compilation.

[nerdzero]

Potential fix incoming. Running through test suite now...

[nerdzero]

I got 1 error which is the test file from the previous patch. Unfortunately, I wasn't able to support the following syntax which the previous patch did:

var someVar as String
    is shared, readonly
    has SomeAttribute
    = "the value"
    """The doc string"""

where the assignment is also indented. I could redo the patch if we want that syntax, but I wanted some feedback on current patch before proceeding any further.

I got 12 other seemingly unrelated errors from testify. Patch and testify output after applying are attached. Add-in compiles with patch as does the example case in ticket 365 (including the one-liner that is commented out). Haven't tried self-hosting.

[Charles]

I have added a TestifyFailures wiki page for reference.

The patch looks fine to me and all things considered, I'm good with it. Hops, do you have any comments or concerns?

[Charles]

Patch applied in changeset:3107

I wanted to proceed so that the XS add-in would build.

If we need to circle back and support the additional case described above, we can still do that.

[hopscc]

Fair enough.

The patch is a good approach to supporting this better.

I think that failing case should be a valid pass tho
- each possible clause nicely laid out, indented on a separate line...

I'll need to recheck the tests now but is there still the possibility of the original error if you have both (and only) an isnames and assignment clause and the assignment precedes the isnames ??

I'll take a look at whats required to support the full clause indented form on the current (patched) codebase..

(good work nerdzero)

[nerdzero]

Replying to hopscc:

(good work nerdzero)

Thanks :) And thanks for laying the groundwork! Otherwise I wouldn't have known where to start.

[hopscc]

New patch to apply on current codebase:
Fixes missing assign-clause on multiline indented and trap for wrong order of ASSIGN then IS-clause so that new message fires...
Reassert previous patch tests removed and add additional for IDE case ( ASSIGN and indented next line has clause - no IS clause). New test for trap for original bug.
good for all unit tests...

[Charles]

I'm thinking that there is no reason why one couldn't have multiple has clauses if the list of attributes was getting long.

[Charles]

Patch applied in changeset:3109

Closing this ticket.

If we decide to support multiple has clauses, we can do a new ticket for that.