(Swift) Letter c is a built-in?

[svanimpe]

In the following snippet:

func max(_ a: Double, _ b: Double, _ c: Double) -> Double {
  if a > b {
    return a > c ? a : c
  } else {
    return b > c ? b : c
  }
}

The parameter c is incorrectly highlighted as a built-in.

[svanimpe]

This seems to result from

highlight.js/src/languages/swift.js

Line 26 in 760dcd6

'bridgeToObjectiveCUnconditional c compactMap contains count countElements countLeadingZeros ' +

No idea what that c is doing there. There is no variable or function with that name in the Standard Library.

[joshgoebel]

I asked @mportiz08 if they know anything about this and whether it could be merged into their PR. If no one knows what it's for then does seem like something we could just yank.

[joshgoebel]

I'd be ok with a PR to remove this it's definitely not a general purpose keyword.

[svanimpe]

@joshgoebel When I finish the project I'm working on, I'll see if I can fix some of the Swift issues I've reported myself.

[svanimpe]

@joshgoebel What do you think about removing the list of built-ins all together?

I looked into updating them, however, I now feel like we should just remove them. In Swift, functions can be overloaded not only on the number and types of their parameters, but also on the return type and on the parameter names. Using only a function's base name to highlight it will lead to many false matches. Proper highlighting would require a type checker.

[joshgoebel]

I now feel like we should just remove them. In Swift, functions can be overloaded not only on the number and types of their parameters, but also on the return type and on the parameter names. Using only a function's base name to highlight it will lead to many false matches.

Removing a large swath of keywords makes auto-detection worse for the whole library, so generally it's frowned upon.

Can you give a few specific examples of the type of issues you're imagining? Or is this c issue truly representative? Typically for something like this we'd add a rule to eliminate false positives. If these are built-in functions could we match then as built_in(...) so there would be no false positives unless the built-in was immediately proceeded by a (, etc?

[svanimpe]

I'm only talking about the global functions in the Standard Library here, not keywords.

The problem is that Swift has now moved most functions into types as methods, and the few functions that remain global often have general names such as min, max, zip, print, stride, sequence etc. These aren't reserved names, so you can declare variables with that name, or even other global functions, as long as they are valid overloads.

For example, here's my best attempt at generating a current list of public global functions in the standard library. These show the number of parameters and their argument labels (_ means unlabeled), but not the parameter types or return type:

 abs(_:)
 all(_:)
 any(_:)
 assert(_:_:file:line:)
 assertionFailure(_:file:line:)
 debugPrint(_:separator:terminator:)
 debugPrint(_:separator:terminator:to:)
 dump(_:name:indent:maxDepth:maxItems:)
 dump(_:to:name:indent:maxDepth:maxItems:)
 fatalError(_:file:line:)
 getVaList(_:)
 isKnownUniquelyReferenced(_:)
 max(_:_:)
 max(_:_:_:_:)
 min(_:_:)
 min(_:_:_:_:)
 numericCast(_:)
 pointwiseMax(_:_:)
 pointwiseMin(_:_:)
 precondition(_:_:file:line:)
 preconditionFailure(_:file:line:)
 print(_:separator:terminator:)
 print(_:separator:terminator:to:)
 readLine(strippingNewline:)
 repeatElement(_:count:)
 sequence(first:next:)
 sequence(state:next:)
 stride(from:through:by:)
 stride(from:to:by:)
 swap(_:_:)
 swift_unboxFromSwiftValueWithType(_:_:)
 transcode(_:_:_:_:stopOnError:)
 transcode(_:from:to:stoppingOnError:into:)
 type(of:)
 unsafeBitCast(_:to:)
 unsafeDowncast(_:to:)
 withExtendedLifetime(_:_:)
 withUnsafeMutablePointer(to:_:)
 withUnsafePointer(to:_:)
 withVaList(_:_:)
 withoutActuallyEscaping(_:do:)
 zip(_:_:)

I don't think a regex grammar can match these correctly, and false positives are extremely confusing for new learners, which is why I'd rather not see them highlighted at all.

[joshgoebel]

I'm only talking about the global functions in the Standard Library here, not keywords.

Yes, I understood.

I don't think a regex grammar can match these correctly, and false positives are extremely confusing for new learners,

It might actually be fun to try with some type of fuzzy look-ahead actually, but I take your larger point.

which is why I'd rather not see them highlighted at all.

Now highlighting them or not is a different question entirely. :-) Though my fear with that line of thinking is that without any visual indication it becomes harder to keep the grammar up-to-date over time (as the language evolves, etc).

[svanimpe]

@joshgoebel Is there a way to tell the parser to only match built-ins if they are followed by an opening brace? I only see one $pattern for all of the keywords.

If not, can I just make a separate mode for these, like this (works for me):

const builtIns = either(/* all options here */);
const BUILT_IN = {
    // TODO: Replace with negative look-behind when available.
    className: 'built_in',
    begin: `(^|[^.])(?=\\b${builtIns}\\()`,
    excludeBegin: true,
    end: `${builtIns}(?=\\()`
};

Also, feel free to assign this to me, I'll fix it anyway now that I have a list of built-ins :)

[joshgoebel]

Is there a way to tell the parser to only match built-ins if they are followed by an opening brace? I only see one $pattern for all of the keywords.

Nope, you'd need a rule for that kind of customization, as you've already realized. beginKeywords already has this . inspection voodoo built-in though....

Also related: #2500

I'd rather see us go ahead and just add a matcher for x.dispatch() though then you won't need all this complexity... you just have one rule to match obj.dispatch() in general, then a second rule to match built-ins.... actually at that point the keyword support may "just work" without needing any other changes.

[svanimpe]

Would you accept the suggested rule above for now? It solves 99% of the issues I have with built-ins being incorrectly highlighted. The remaining 1% would be overloads with the same name as a built-in, which I don't think we can detect with regexes.

[joshgoebel]

I suggested what I did because often that's how we prefer to solve this. For example see JS:

      // hack: prevents detection of keywords in some circumstances
      // .keyword()
      // $keyword = x
      {
        variants: [
          { begin: '\\.' + IDENT_RE },
          { begin: '\\$' + IDENT_RE }
        ],
        relevance: 0
      },

This is much easier to understand (since you're version would even be simpler):

{ // consume .built_in to prevent highlight
  begin: concat(/\./, either(...builtIns), /\(/)
  relevance: 0,
}

A ruleset with more rules but simpler rules is far better than a rule set with fewer but very complex rules. And again this is also very compatible with the future direction to turn this into a "dispatch" rule... in fact you might not even need builtIns there at all vs just IDENT of some kind, etc.

[svanimpe]

Fixed in #2908