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] Undo parsing function calls with trailing closure #862

Merged
merged 1 commit into from
Jan 23, 2018

Conversation

dan-zheng
Copy link
Contributor

@dan-zheng dan-zheng commented Jan 11, 2018

Patches some type name bugs, addressing #861.
Fixed examples:
2018-01-11 at 6 53 33 am
2018-01-11 at 7 04 02 am
2018-01-11 at 7 04 19 am

Edit: after careful consideration, I decided to revert parsing function calls with trailing closure.
It's not possible to perfectly identify such function calls using the current lexer and the feature causes more harm than good.

@dan-zheng dan-zheng changed the title [Swift] Fix type name bugs [Swift] [DO NOT MERGE] Fix type name bugs Jan 11, 2018
@dan-zheng dan-zheng changed the title [Swift] [DO NOT MERGE] Fix type name bugs [Swift] [WIP] Fix type name bugs Jan 11, 2018
@dan-zheng dan-zheng changed the title [Swift] [WIP] Fix type name bugs [Swift] Fix type name bugs Jan 11, 2018
@dan-zheng
Copy link
Contributor Author

Done patching and ready for merging.
Generic functions are also parsed correctly now.

Before:
2018-01-11 at 8 34 12 am

After:
2018-01-11 at 8 33 20 am

@johnfairh
Copy link
Contributor

Thanks for looking!

It would be worth your checking the html diffs of visual/samples/swift between Rouge 3.0.0 and this change: there are still quite a few false-positive differences where 'identifier-followed-by-{<' are unintentionally matching as function names.

Easiest is keywords like else { and didSet {. Basically anything that can precede a {.

Trickier than that, there are cases to think about like (not claiming exhaustive list):

if a && b { // 'b' looks like a function name by the '{' rule
if let a = b { // same
if a < b { // 'a' looks like func name by the '<' rule, 'b' also by the '{' rule
switch A(b).c { // 'c' looks like a function name
for _ in 1...amountToPad { // 'amountToPad' looks like a function name

infix operator <> : ComparisonPrecedence // 'operator' looks like a function name by the '<' rule

In some of these it's ambiguous whether the identifier is being used as a function call or not without the rest of the statement. I think this might be a limitation of how this lexer is set up, not sure there is a perfect solution without a bigger rewrite to fully deal with Swift grammar taking much more context into account.

@dan-zheng
Copy link
Contributor Author

Wow, thanks for the thorough analysis!
Yes, I agree that a perfect solution would require taking context into account - otherwise it's generally impossible to distinguish variable names/function names/type names. A halfway solution could be to maintain longer lists of built-in types and functions, but it's not really robust.

I suppose I should partially roll back #837 and undo function calls with lambda argument since that seems to cause more harm than good.

 - Parsing trailing closures beginning with '{' is too tricky without
 context and causes more harm than good
@dan-zheng dan-zheng changed the title [Swift] Fix type name bugs [Swift] Undo parsing function calls with trailing closure Jan 11, 2018
@johnfairh
Copy link
Contributor

Sounds good.

@gfx
Copy link
Member

gfx commented Jan 23, 2018

Thanks! It looks good.

@gfx gfx merged commit 367e989 into rouge-ruby:master Jan 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants