textmate / swift.tmbundle

TextMate support for Swift
72 stars 30 forks source link

Allow restricted setter access control modifiers #27

Closed patrickrgaffney closed 6 years ago

patrickrgaffney commented 7 years ago

This commit allows the internal, private, and fileprivate access modifiers to restrict the write scope of a variable, property, or subscript by using the (set) syntax. See the Getters and Setters section of The Swift Programming Language for more details.

It fixes the following issues:

fileprivate(set) var laps: [TimeInterval] = []
private(set) var dates: [Date] = []
internal(set) var string: String?
jtbandes commented 7 years ago

Is the goal to make fileprivate(set) all be part of the same scope? That more closely matches in appearance what Xcode does, I believe, but @infininight had intentionally made attributes and compound names look more like this:

@available(deprecated)
foo(bar:)

I think it's somewhat desirable for the parens to be marked as punctuation.*, but other than that I don't have a strong opinion (that wouldn't exclude them being colored the same as the rest of the keyword).

patrickrgaffney commented 7 years ago

Yeah, my goal was to make it more like Xcode.

Using the current syntax, the set portion of the access modifier gets highlighted as a function call — rightfully so — and the internal, fileprivate, or private gets highlighted as a keyword. I use Xcode a lot, so it seemed off to me, but I can see both sides.

jtbandes commented 7 years ago

So it looks like for @attribute(args) we have punctuation.definition.arguments.{begin,end} for the parens, and meta.arguments.attribute for the arguments. How about doing the same for accessibility modifiers, i.e. the same thing for parens and meta.arguments.accessibility for the arguments?

patrickrgaffney commented 7 years ago

That sounds like a good idea.

I have been using this for the past few days:

{   name = 'meta.accessiblity-modifiers.swift';
        comment = 'provides lower access levels for getters';
    begin = '(?<!\.)\b(?:(fileprivate|private|internal))\b(\()';
    end = '(\))';
    beginCaptures = {
        1 = { name = 'keyword.other.accessiblity-modifiers.swift'; };
        2 = { name = 'punctuation.definition.arguments.begin.swift'; };
    };
    endCaptures = { 0 = { name = 'punctuation.definition.arguments.end.swift'; }; };
    patterns = (
        {   match = '\b(set)\b';
            captures = {
                1 = { name = 'keyword.other.accessiblity-modifiers.arguments.swift'; };
            };
        },
    );
},

It appears to work — let me know what you think.

jtbandes commented 7 years ago

I would say in this case we can just do a single pattern, like

(?<!\.)\b(fileprivate|private|internal|public|open)\s*(?:(\()(set)(\)))?
patrickrgaffney commented 7 years ago

Okay, this should work — although it probably needs to be rebase'd.

I changed the regex slightly to only properly scope the parenthesis and arguments for fileprivate, private, and internal, as those are the only modifiers that these restricted setters work for. The public and open modifiers will still get the highlight, but the scope for the set will be a function call instead of keyword.other.accessiblity-modifier.arguments.swift.

jtbandes commented 7 years ago

Looking at the grammar it seems like public(set) and open(set) are allowed too: https://developer.apple.com/library/content/documentation/Swift/Conceptual/Swift_Programming_Language/zzSummaryOfTheGrammar.html#//apple_ref/doc/uid/TP40014097-CH38-ID477

Maybe I'm missing something though?

Also, could you squash your commits since it's just one single change? Looks like you have some merge conflicts in there currently.

patrickrgaffney commented 7 years ago

Okay, fixed the commits. I just got a way-to-expensive iTouchBar and moving between two computers at once is apparently quite a challenge for me.

As far as the grammar goes... that's bizarre. The compiler will give you an error if you attempt the following:

screen shot 2016-12-03 at 4 58 30 pm

And this backs up what the Language Guide states:

You can give a setter a lower access level than its corresponding getter, to restrict the read-write scope of that variable, property, or subscript. You assign a lower access level by writing fileprivate(set), private(set), or internal(set) before the var or subscript introducer.

Under that logic it wouldn't really make sense to have public(set). But it is in the grammar... 🤔

jtbandes commented 7 years ago

Hmm, I wonder if open open(set) var works?

patrickrgaffney commented 7 years ago

You are correct, the following combinations emit no errors:

open public(set) var integer = 1
public public(set) var integer = 1
open open(set) var integer = 1 
public open(set) var integer = 1 

I'll be damned.