Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Swift) params does not handled nested parens properly #2857

Closed
svanimpe opened this issue Nov 13, 2020 · 8 comments · Fixed by #2930
Closed

(Swift) params does not handled nested parens properly #2857

svanimpe opened this issue Nov 13, 2020 · 8 comments · Fixed by #2930
Labels
bug help welcome Could use help from community language

Comments

@svanimpe
Copy link
Contributor

In the following example:

func sizeOfRectangle(from corner1: (x: Double, y: Double), to corner2: (x: Double, y: Double)) -> (width: Double, height: Double) {
  let width = abs(corner2.x - corner1.x)
  let height = abs(corner2.y - corner1.y)
  return (width, height)
}

Only the part from corner1: (x: Double, y: Double) is highlighted as a parameter list. I assume the grammar thinks the closing parenthesis of the tuple is the closing parenthesis of the parameter list?

The grammar also doesn't support line breaks in parameter lists, so only the first line is highlighted.

I'm also wondering why the parameter list is highlighted at all? It can be really complex in Swift, so it doesn't make sense to me that it's just one color.

@svanimpe svanimpe added bug help welcome Could use help from community language labels Nov 13, 2020
@joshgoebel
Copy link
Member

I assume the grammar thinks the closing parenthesis of the tuple is the closing parenthesis of the parameter list?

No, it's because params has endsParent so the nested params (any inside () group is considered more params [though it should really be unnamed] terminates as soon as the first ) is hit because endParent fires for both and walks back up the mode tree.

To do this right you typically need a new mode. If we had sugar for it it might look like this:

          {  className: 'params',
            begin: /\(/, end: /\)/, endsParent: true,
            contains: [
              {'self', endsParent: false }, // this is magic sugar that doesn't exist plus being invalid JS, lol
              NUMBER,

So one has to have two modes OR remove the endsParent... I'm not sure what it's intended to do. I mean I know what it's intended to do (end the function) but not why it's needed since the { will end the function anyways... unless perhaps you can have function declarations without bodies? Many languages do this same trick but don't require endsParent. First time I've seen it actually.

The grammar also doesn't support line breaks in parameter lists, so only the first line is highlighted.

It most certainly does, I'm not sure what you're seeing. There is no special handling of line breaks anywhere though. The function is either terminated by { or by the closing ) of any params group (or a few illegal characters).

I'm also wondering why the parameter list is highlighted at all? It can be really complex in Swift, so it doesn't make sense to me that it's just one color.

This is something we traditionally highlight... it's up to themes to decide what to do with it... a theme that turns it all the same color probably isn't the best choice for Swift. Our parsing is currently a tree though so it's possible for themes to style params > variable differently than variable might appear elsewhere. If we were going to consider changing this for individual languages I'd want to first look at all the themes and see if any of them target things inside of param groups. Right now OTTOMH only Atom One Reasonable seems to do this for ReasonML:

.hljs-function .hljs-params .hljs-typing {
  color: #FD971F;
}

I'd be open to getting into a larger discussion about this though. params is helpful for many languages because we can't/don't get into detail about what's inside params so in many simpler cases just blanketing the whole area with a class works nicely. The more complex a language the more this starts to fall apart.

@joshgoebel
Copy link
Member

joshgoebel commented Nov 13, 2020

Any time we're talking visual stuff screen grabs are often nice. :-)

Are types and classes the same thing in Swift? I would imagine in some languages they are different so having the context of "we're in params now" could help with knowing how to highlight identifiers, etc...

@joshgoebel joshgoebel changed the title (Swift) Grammar doesn't support tuple parameters (Swift) params does not handled nested parens properly Nov 13, 2020
@svanimpe
Copy link
Contributor Author

svanimpe commented Nov 15, 2020

Here's a screenshot:

Screen Shot 2020-11-15 at 19 35 17

The "line break" issue was just me being confused. It's just the closing parenthesis again that ends the highlighting, not the line break.

Are types and classes the same thing in Swift?

No, Swift also has struct and enum types.

@joshgoebel
Copy link
Member

joshgoebel commented Nov 15, 2020

It can be really complex in Swift, so it doesn't make sense to me that it's just one color.

What are some of the more complex cases? Can I write 100 lines of code and insert an inline function as a default argument or something crazy?

Also, it truly shouldn't "all be one color"... in the best case the grammar should be enhanced to add some nuance... highlight types, argument names, etc... the question is do we need the context of params to do that properly. I feel like SOME of that content should indeed be highlighted as params, no?

For example here is the type of highlighting commonly seen:

Screen Shot 2020-11-15 at 1 51 21 PM

So for Swift perhaps only to from or corner1 and corner2 should be highlighted as params? What would this look like in XCode, say?

@joshgoebel
Copy link
Member

joshgoebel commented Nov 15, 2020

Screen Shot 2020-11-15 at 1 54 50 PM

Just adding types is already much better.

@svanimpe
Copy link
Contributor Author

Also leaving this here as a reminder for whoever picks this up:

Screen Shot 2020-11-15 at 22 28 28

Closures don't seem to be supported w.r.t. parameter highlighting?

@svanimpe
Copy link
Contributor Author

Here's a screenshot from VS Code, which uses a TextMate grammar I believe:

Screen Shot 2020-11-15 at 22 31 38

Xcode only highlights the types, not the parameter names (same for closures):

Screen Shot 2020-11-15 at 22 33 26

@joshgoebel
Copy link
Member

Closures don't seem to be supported w.r.t. parameter highlighting?

I'd guess not, typically we haven't supported highlighting function dispatch - though we're open to that now if it can be detected.

joshgoebel pushed a commit that referenced this issue Dec 28, 2020
…ipts (#2930)

This PR improves highlighting for functions, initializers, and subscripts. This includes detailed highlighting for generic parameters as well as function parameters.

To differentiate between tuple elements and function params, I've also added a mode to match tuples, whose element names should not be highlighted as params.

- Fixes #2895
- Fixes #2857
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug help welcome Could use help from community language
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants