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

Improve Swift declarations using SourceKit's version #902

Merged
merged 1 commit into from
Apr 4, 2018
Merged

Conversation

johnfairh
Copy link
Collaborator

@johnfairh johnfairh commented Nov 7, 2017

Still working through specs and tweaking things, appreciate feedback about any of this especially around how @attributes are handled.

This changes Swift declarations to use the compiler’s round-tripped version instead of the raw parsed source code in many cases. Improvements:

  • Show { get } / { get set } for properties in protocols, for subscripts, and for computed properties
  • If code says var fs = “Fred” then show decl var fs: String
  • Don’t show unwanted code fragments that confused the sourcekitten parser (eg. Lazy properties documentation includes unneeded code #768)
  • Show all declaration attributes (eg. @discardable, @objc, @available)
  • Standardize formatting (eg. whitespace)

Still prefer the parsed declaration when it is better:

  • Functions with default parameters. Values not available from compiler, SR-2608, perhaps fixed in Swift 5.
  • Declarations arranged over multiple lines - presume the user has formatted these nicely so keep them. Should fix to be adaptive in Jazzy html/js some day.
  • A ?bug to do with @escaping / @autoclosure - SR 6321, try + fix for Swift 4.1.

Re. decl attributes, I’ve chosen to stack these each on their own line, to avoid very long lines. I think they’re all useful in docs --- API users may legitimately want to know these? Eg:

screen shot 2017-10-27 at 14 45 55

screen shot 2017-10-27 at 15 02 36

screen shot 2017-10-27 at 18 05 11
[this last fabricated of course but you get the idea...]

Jazzy should handle @available in a more visual way but for now I think it’s an improvement to pass through what the author wrote? The highlighting is a bit ugly; there’s a rouge PR to improve it.

Notes on spec changes in the specs PR.

@johnfairh
Copy link
Collaborator Author

Going over the specs this all looks fine with various fixes and improvements. One part of 'standardise formatting' to call out: generic parameter constraints are always displayed in the where clause. So source code:
screen shot 2017-11-10 at 10 56 39
becomes:
screen shot 2017-11-10 at 10 55 42

@johnfairh johnfairh changed the title [wip] Improve Swift declarations using SourceKit's version Improve Swift declarations using SourceKit's version Nov 10, 2017
@jpsim
Copy link
Collaborator

jpsim commented Nov 14, 2017

So we used to do this a long time ago, but lost too much of the formatting from the original source to be worthwhile.

Also, I think there are printing options for SourceKit (at least internally to the Swift compiler) that allow printing out default parameter values rather than just default: https://github.com/apple/swift/blob/39c3fae1df18f2804b03b37c4446b57ca20effb2/include/swift/AST/PrintOptions.h#L145-L146

@johnfairh
Copy link
Collaborator Author

Yes I kept finding small problems needing workarounds; thought I'd keep going though.

That print option just controls whether you get = default or nothing at all :/ To make it work well I think we need Swift 5 [+] to move the default expression into the module interface, currently they're not in the serialised AST.

@jpsim
Copy link
Collaborator

jpsim commented Jan 8, 2018

@johnfairh what would you like to do with this? If it's not a 100% improvement on existing behavior, but improves some aspects, I wouldn't be opposed to having this behavior as a configuration flag.

Let me know if you want me to review again.

@johnfairh
Copy link
Collaborator Author

@jpsim I think the changes this PR makes are all improvements. If it spots a declaration where the compiler's version is worse (eg. default params,) then it uses the parsed version as today.

Let me rebase + retest...

@johnfairh
Copy link
Collaborator Author

Fiiiiinally back to this. Reworked code a bit to fix the two problems from before:

  • All attributes are now picked up even if parsed version of declaration is used;
  • Worked around Rouge formatting bug with @available + quoted strings + parens

Worth a review @jpsim when you have a chance -- I think it improves everything.

@jpsim jpsim self-assigned this Apr 1, 2018
Copy link
Collaborator

@jpsim jpsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've sat on this for long enough. @johnfairh you've accomplished a ton of awesome improvements in this and I think you should merge 😄.

...except when the parsed one has more information or looks
better.  Notes on the various declarations from sourcekitten:

[fully_]annotated_declaration - present if the declaration is
compiled (not #if'd out).  'Quick Help' print options, includes
all attributes except @available.

doc.declaration - from the full_as_xml, so only if there is a doc
comment + the decl is compiled.  'Interface' print options.  Subtle
differences to annotated_declaration: does include @available, does
not include @discardableResult, ignores `deinit`.

parsed_declaration - from source code, omits attributes on lines
above the start of the Swift decl.
@johnfairh johnfairh merged commit 9cc5b2c into master Apr 4, 2018
@johnfairh johnfairh deleted the jf-decls2 branch April 4, 2018 10:55
@johnfairh
Copy link
Collaborator Author

Thanks - merging. Code here definitely benefited from the gestation period.

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.

2 participants