Skip to content

DNM: Fix up effect specifiers in function signatures #889

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.

Sign up for GitHub

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

Closed
wants to merge 1 commit into from
Closed

DNM: Fix up effect specifiers in function signatures #889

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
168 changes: 90 additions & 78 deletions Sources/SwiftParser/Declarations.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1440,26 +1440,18 @@ extension Parser {
arena: self.arena)
}

/// If a `throws` keyword appears right in front of the `arrow`, it is returned as `misplacedThrowsKeyword` so it can be synthesized in front of the arrow.
@_spi(RawSyntax)
public mutating func parseFunctionReturnClause() -> (returnClause: RawReturnClauseSyntax, misplacedThrowsKeyword: RawTokenSyntax?) {
let (unexpectedBeforeArrow, arrow) = self.expect(.arrow)
var misplacedThrowsKeyword: RawTokenSyntax? = nil
let unexpectedBeforeReturnType: RawUnexpectedNodesSyntax?
if let throwsKeyword = self.consume(ifAny: [.rethrowsKeyword, .throwsKeyword]) {
misplacedThrowsKeyword = throwsKeyword
unexpectedBeforeReturnType = RawUnexpectedNodesSyntax(elements: [RawSyntax(throwsKeyword)], arena: self.arena)
} else {
unexpectedBeforeReturnType = nil
}
/// If effect specifiers appear in front of the `arrow` or after the return type, they are returned so that they can be synthesized in front of the arrow.
mutating func parseReturnClause(_ handle: RecoveryConsumptionHandle) -> (returnClause: RawReturnClauseSyntax, misplacedSpecifiers: [SpecifierWithToken]) {
let (unexpectedBeforeArrow, arrow) = self.eat(handle)
let misplacedSpecifiers = parseAllEffectsSpecifiers()
let result = self.parseResultType()
let returnClause = RawReturnClauseSyntax(
unexpectedBeforeArrow,
arrow: arrow,
unexpectedBeforeReturnType,
RawUnexpectedNodesSyntax(misplacedSpecifiers, arena: self.arena),
returnType: result,
arena: self.arena)
return (returnClause, misplacedThrowsKeyword)
return (returnClause, misplacedSpecifiers)
}
}

Expand Down Expand Up @@ -1544,33 +1536,27 @@ extension Parser {
public mutating func parseFunctionSignature() -> RawFunctionSignatureSyntax {
let input = self.parseParameterClause(for: .functionParameters)

let async: RawTokenSyntax?
if let asyncTok = self.consumeIfContextualKeyword("async") {
async = asyncTok
} else if let reasync = self.consumeIfContextualKeyword("reasync") {
async = reasync
} else {
async = nil
}

var throwsKeyword = self.consume(ifAny: [.throwsKeyword, .rethrowsKeyword])
var (asyncOrReasync, throwsOrRethrows, specifiersAfterThrows) = parseEffectsSpecifiers(atDecl: true)

let output: RawReturnClauseSyntax?
if self.at(.arrow) {
let result = self.parseFunctionReturnClause()
output = result.returnClause
if let misplacedThrowsKeyword = result.misplacedThrowsKeyword, throwsKeyword == nil {
throwsKeyword = RawTokenSyntax(missing: misplacedThrowsKeyword.tokenKind, arena: self.arena)
}
if let handle = self.canRecoverTo(.arrow) {
let (returnClause, misplacedSpecifiers) = parseReturnClause(handle)
addMissingEffectsSpecifiers(atDecl: true, from: misplacedSpecifiers, asyncOrReasync: &asyncOrReasync, throwsOrRethrows: &throwsOrRethrows)
output = returnClause
} else {
output = nil
}

let specifiersAfterReturn = parseAllEffectsSpecifiers()
addMissingEffectsSpecifiers(atDecl: true, from: specifiersAfterReturn, asyncOrReasync: &asyncOrReasync, throwsOrRethrows: &throwsOrRethrows)

return RawFunctionSignatureSyntax(
input: input,
asyncOrReasyncKeyword: async,
throwsOrRethrowsKeyword: throwsKeyword,
asyncOrReasyncKeyword: asyncOrReasync,
throwsOrRethrowsKeyword: throwsOrRethrows,
RawUnexpectedNodesSyntax(specifiersAfterThrows, arena: self.arena),
output: output,
RawUnexpectedNodesSyntax(specifiersAfterReturn, arena: self.arena),
arena: self.arena)
}
}
Expand Down Expand Up @@ -1601,9 +1587,11 @@ extension Parser {

let indices = self.parseParameterClause(for: .indices)

let unexpectedBeforeArrow = RawUnexpectedNodesSyntax(parseAllEffectsSpecifiers(), arena: self.arena)

let result: RawReturnClauseSyntax
if self.at(.arrow) {
result = self.parseFunctionReturnClause().returnClause
if let handle = self.canRecoverTo(.arrow) {
result = self.parseReturnClause(handle).returnClause
Copy link
Member

Choose a reason for hiding this comment

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

You are dropping the effect specifiers here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Weird that I can't see this in the review. But no, they're added to the return clause itself. They're only returned to be able to make async/throws missing when necessary. This is one of the many reasons I'm not a huge fan of this patch (though this particular returning of unexpected nodes was already here).

It really seems like we should just recover + place tokens in unexpected + have ASTGen handle these diagnostics. Though one issue there is that they wouldn't be parsed as contextual keywords if we just did regular recovery vs looked for the effects specifically.

@unknown is sort of an example of this, though it currently allows any attribute as the @unknown attribute and parses the rest as unexpected (ie. just recovers to the case/default).

Copy link
Member

Choose a reason for hiding this comment

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

Ah, never mind. I missed that throws also gets added to the unexpected tokens of the returnClause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, but it's super easy to miss. Hence my problem with doing it this way :)

} else {
result = RawReturnClauseSyntax(
arrow: RawTokenSyntax(missing: .arrow, arena: self.arena),
Expand Down Expand Up @@ -1635,6 +1623,7 @@ extension Parser {
subscriptKeyword: subscriptKeyword,
genericParameterClause: genericParameterClause,
indices: indices,
unexpectedBeforeArrow,
result: result,
genericWhereClause: genericWhereClause,
accessor: accessor,
Expand Down Expand Up @@ -1767,41 +1756,6 @@ extension Parser {
attributes: attrs, modifier: modifier, kind: kind, token: introducer)
}

@_spi(RawSyntax)
public mutating func parseEffectsSpecifier() -> RawTokenSyntax? {
// 'async'
if let async = self.consumeIfContextualKeyword("async") {
return async
}

// 'reasync'
if let reasync = self.consumeIfContextualKeyword("reasync") {
return reasync
}

// 'throws'/'rethrows'
if let throwsRethrows = self.consume(ifAny: [.throwsKeyword, .rethrowsKeyword]) {
return throwsRethrows
}

// diagnose 'throw'/'try'.
if let throwTry = self.consume(ifAny: [.throwKeyword, .tryKeyword], where: { !$0.isAtStartOfLine }) {
return throwTry
}

return nil
}

@_spi(RawSyntax)
public mutating func parseEffectsSpecifiers() -> [RawTokenSyntax] {
var specifiers = [RawTokenSyntax]()
var loopProgress = LoopProgressCondition()
while let specifier = self.parseEffectsSpecifier(), loopProgress.evaluate(currentToken) {
specifiers.append(specifier)
}
return specifiers
}

/// Parse the body of a variable declaration. This can include explicit
/// getters, setters, and observers, or the body of a computed property.
///
Expand Down Expand Up @@ -1884,14 +1838,15 @@ extension Parser {

// Next, parse effects specifiers. While it's only valid to have them
// on 'get' accessors, we also emit diagnostics if they show up on others.
let asyncKeyword: RawTokenSyntax?
let throwsKeyword: RawTokenSyntax?
if self.at(anyIn: EffectsSpecifier.self) != nil {
asyncKeyword = self.parseEffectsSpecifier()
throwsKeyword = self.parseEffectsSpecifier()
var asyncOrReasync: RawTokenSyntax? = nil
var throwsOrRethrows: RawTokenSyntax? = nil
let unexpectedAfterThrows: [SpecifierWithToken]
if introducer.kind == .get {
(asyncOrReasync, throwsOrRethrows, unexpectedAfterThrows) = parseEffectsSpecifiers(atDecl: true)
} else {
asyncKeyword = nil
throwsKeyword = nil
asyncOrReasync = nil
throwsOrRethrows = nil
unexpectedAfterThrows = parseAllEffectsSpecifiers()
}

let body = self.parseOptionalCodeBlock()
Expand All @@ -1901,8 +1856,9 @@ extension Parser {
modifier: introducer.modifier,
accessorKind: introducer.token,
parameter: parameter,
asyncKeyword: asyncKeyword,
throwsKeyword: throwsKeyword,
asyncKeyword: asyncOrReasync,
throwsKeyword: throwsOrRethrows,
RawUnexpectedNodesSyntax(unexpectedAfterThrows, arena: self.arena),
body: body,
arena: self.arena))
}
Expand Down Expand Up @@ -2233,3 +2189,59 @@ extension Parser {
}
}
}

extension Parser {
mutating func parseAllEffectsSpecifiers() -> [SpecifierWithToken] {
var specifiers = [SpecifierWithToken]()
var loopProgress = LoopProgressCondition()
while let (specifier, handle) = self.canRecoverTo(anyIn: EffectsSpecifier.self), loopProgress.evaluate(currentToken) {
let (unexpected, token) = self.eatKeepUnexpected(handle)
specifiers.append(contentsOf: unexpected.map { SpecifierWithToken(specifier: nil, token: $0) })
Copy link
Member

Choose a reason for hiding this comment

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

I don’t really like how we are considering unexpected tokens as SpecifierWithToken here and setting specifier to nil.

What do you think about changing SpecifierWithToken to

struct ParsedEffectsSpecifier {
  let specifier: EffectsSpecifier?
  let unexpected: RawUnexpectedNodesSyntax?
  let token: RawTokenSyntax
 }

That way you also don’t need to modify eat.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much better name, but I'm not sure I understand the unexpected suggestion. The issue here is that they all need to get merged into the single RawUnexpectedNodeSyntax.

Copy link
Member

Choose a reason for hiding this comment

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

We can add RawUnexpectedNodeSyntax as a child of RawUnexpectedNodeSyntax. Or, if you don’t like that, maybe it should be

struct ParsedEffectsSpecifier {
  let specifier: EffectsSpecifier?
  let unexpected: [RawTokenSyntax]
  let token: RawTokenSyntax
 }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, makes sense.

specifiers.append(SpecifierWithToken(specifier: specifier, token: token))
}
return specifiers
}

func addMissingEffectsSpecifiers(atDecl: Bool, from unexpectedSpecifiers: [SpecifierWithToken], asyncOrReasync: inout RawTokenSyntax?, throwsOrRethrows: inout RawTokenSyntax?) {
for unexpected in unexpectedSpecifiers {
guard asyncOrReasync == nil || throwsOrRethrows == nil else {
break
}

if asyncOrReasync == nil, unexpected.specifier == .asyncContextualKeyword || (atDecl && unexpected.specifier == .reasyncContextualKeyword) {
asyncOrReasync = RawTokenSyntax(missing: unexpected.token.tokenKind, text: unexpected.token.tokenText, arena: self.arena)
}

if throwsOrRethrows == nil, unexpected.specifier == .throwKeyword || (atDecl && unexpected.specifier == .rethrowsKeyword) {
throwsOrRethrows = RawTokenSyntax(missing: unexpected.token.tokenKind, arena: self.arena)
}
}
}

mutating func parseEffectsSpecifiers(atDecl: Bool) -> (asyncOrReasync: RawTokenSyntax?,
throwsOrRethrows: RawTokenSyntax?,
specifiersAfterThrows: [SpecifierWithToken]){
var initialSpecifiers = parseAllEffectsSpecifiers()

var asyncOrReasync: RawTokenSyntax?
var throwsOrRethrows: RawTokenSyntax?
if !initialSpecifiers.isEmpty {
let first = initialSpecifiers.removeFirst()
if first.specifier == .asyncContextualKeyword || atDecl && first.specifier == .reasyncContextualKeyword {
asyncOrReasync = first.token

if !initialSpecifiers.isEmpty {
let second = initialSpecifiers.removeFirst()
if second.specifier == .throwsKeyword || atDecl && second.specifier == .rethrowsKeyword {
throwsOrRethrows = second.token
}
}
} else if first.specifier == .throwsKeyword || atDecl && first.specifier == .rethrowsKeyword {
throwsOrRethrows = first.token
}
addMissingEffectsSpecifiers(atDecl: atDecl, from: initialSpecifiers, asyncOrReasync: &asyncOrReasync, throwsOrRethrows: &throwsOrRethrows)
}

return (asyncOrReasync, throwsOrRethrows, initialSpecifiers)
}
}
31 changes: 15 additions & 16 deletions Sources/SwiftParser/Expressions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2066,8 +2066,9 @@ extension Parser {
}

var input: RawSyntax?
var asyncKeyword: RawTokenSyntax? = nil
var asyncTok: RawTokenSyntax? = nil
var throwsTok: RawTokenSyntax? = nil
var specifiersAfterThrows = [SpecifierWithToken]()
var output: RawReturnClauseSyntax? = nil
if !self.at(.inKeyword) {
if self.at(.leftParen) {
Expand Down Expand Up @@ -2097,32 +2098,30 @@ extension Parser {
input = RawSyntax(RawClosureParamListSyntax(elements: params, arena: self.arena))
}

asyncKeyword = self.parseEffectsSpecifier()
throwsTok = self.parseEffectsSpecifier()
(asyncTok, throwsTok, specifiersAfterThrows) = parseEffectsSpecifiers(atDecl: false)

// Parse the optional explicit return type.
if let arrow = self.consume(if: .arrow) {
// Parse the type.
let returnTy = self.parseType()

output = RawReturnClauseSyntax(
arrow: arrow,
returnType: returnTy,
arena: self.arena
)
if let handle = self.canRecoverTo(.arrow) {
let (returnClause, misplacedSpecifiers) = parseReturnClause(handle)
addMissingEffectsSpecifiers(atDecl: false, from: misplacedSpecifiers, asyncOrReasync: &asyncTok, throwsOrRethrows: &throwsTok)
output = returnClause
}
}

var specifiersBeforeIn: [SpecifierWithToken] = parseAllEffectsSpecifiers()
addMissingEffectsSpecifiers(atDecl: false, from: specifiersBeforeIn, asyncOrReasync: &asyncTok, throwsOrRethrows: &throwsTok)

// Parse the 'in'.
let (unexpectedBeforeInTok, inTok) = self.expect(.inKeyword)
let (unexpectedBeforeInTok, inTok) = self.expectKeepUnexpected(.inKeyword)

return RawClosureSignatureSyntax(
attributes: attrs,
capture: captures,
input: input,
asyncKeyword: asyncKeyword,
asyncKeyword: asyncTok,
throwsTok: throwsTok,
RawUnexpectedNodesSyntax(specifiersAfterThrows, arena: self.arena),
output: output,
unexpectedBeforeInTok,
RawUnexpectedNodesSyntax(specifiersBeforeIn.map { $0.token } + unexpectedBeforeInTok, arena: self.arena),
inTok: inTok,
arena: self.arena)
}
Expand Down
5 changes: 5 additions & 0 deletions Sources/SwiftParser/Lookahead.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,11 @@ extension Parser {
public func lookahead() -> Lookahead {
return Lookahead(cloning: self)
}

public func lookahead<T>(_ body: (_: inout Lookahead) -> T) -> T {
var lookahead = lookahead()
return body(&lookahead)
}
}

extension Parser.Lookahead {
Expand Down
62 changes: 49 additions & 13 deletions Sources/SwiftParser/Parser.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,40 +281,76 @@ extension Parser {
}

/// Eat a token that we know we are currently positioned at, based on `canRecoverTo(anyIn:)`.
mutating func eat(_ handle: RecoveryConsumptionHandle) -> (RawUnexpectedNodesSyntax?, Token) {
let unexpectedNodes: RawUnexpectedNodesSyntax?
mutating func eatKeepUnexpected(_ handle: RecoveryConsumptionHandle) -> (unexpected: [RawTokenSyntax], token: Token) {
var unexpectedTokens = [RawTokenSyntax]()
if handle.unexpectedTokens > 0 {
var unexpectedTokens = [RawSyntax]()
for _ in 0..<handle.unexpectedTokens {
unexpectedTokens.append(RawSyntax(self.consumeAnyToken()))
unexpectedTokens.append(self.consumeAnyToken())
}
unexpectedNodes = RawUnexpectedNodesSyntax(elements: unexpectedTokens, arena: self.arena)
} else {
unexpectedNodes = nil
}
let token = self.eat(handle.tokenConsumptionHandle)
return (unexpectedNodes, token)
return (unexpectedTokens, token)
}

/// Eat a token that we know we are currently positioned at, based on `canRecoverTo(anyIn:)`.
mutating func eat(_ handle: RecoveryConsumptionHandle) -> (unexpected: RawUnexpectedNodesSyntax?, token: Token) {
let (unexpected, token) = eatKeepUnexpected(handle)
return (RawUnexpectedNodesSyntax(elements: unexpected.map(RawSyntax.init), arena: self.arena), token)
}
}

// MARK: Expecting Tokens with Recovery

extension Parser {
/// Implements the paradigm shared across all `expect` methods.
private mutating func expectImpl(
private mutating func expectImplKeepUnexpected(
consume: (inout Parser) -> RawTokenSyntax?,
canRecoverTo: (inout Lookahead) -> RecoveryConsumptionHandle?,
makeMissing: (inout Parser) -> RawTokenSyntax
) -> (unexpected: RawUnexpectedNodesSyntax?, token: RawTokenSyntax) {
) -> (unexpected: [RawTokenSyntax], token: RawTokenSyntax) {
if let tok = consume(&self) {
return (nil, tok)
return ([], tok)
}
var lookahead = self.lookahead()
if let handle = canRecoverTo(&lookahead) {
let (unexpectedTokens, token) = self.eat(handle)
let (unexpectedTokens, token) = self.eatKeepUnexpected(handle)
return (unexpectedTokens, token)
}
return (nil, makeMissing(&self))
return ([], makeMissing(&self))
}

/// Implements the paradigm shared across all `expect` methods.
private mutating func expectImpl(
consume: (inout Parser) -> RawTokenSyntax?,
canRecoverTo: (inout Lookahead) -> RecoveryConsumptionHandle?,
makeMissing: (inout Parser) -> RawTokenSyntax
) -> (unexpected: RawUnexpectedNodesSyntax?, token: RawTokenSyntax) {
let (unexpected, token) = expectImplKeepUnexpected(consume: consume, canRecoverTo: canRecoverTo, makeMissing: makeMissing)
return (RawUnexpectedNodesSyntax(unexpected, arena: self.arena), token)
}

/// Attempts to consume a token of the given kind.
/// If it cannot be found, the parser tries
/// 1. To eat unexpected tokens that have lower ``TokenPrecedence`` than the
/// expected token and see if the token occurs after that unexpected.
/// 2. If the token couldn't be found after skipping unexpected, it synthesizes
/// a missing token of the requested kind.
@_spi(RawSyntax)
public mutating func expectKeepUnexpected(
_ kind: RawTokenKind,
remapping: RawTokenKind? = nil
) -> (unexpected: [RawTokenSyntax], token: RawTokenSyntax) {
return expectImplKeepUnexpected(
consume: { $0.consume(if: kind, remapping: remapping) },
canRecoverTo: { $0.canRecoverTo([kind]) },
makeMissing: {
if let remapping = remapping {
return $0.missingToken(remapping, text: kind.defaultText)
} else {
return $0.missingToken(kind, text: nil)
}
}
)
}

/// Attempts to consume a token of the given kind.
Expand Down
Loading