Async deinit diagnostics in swift-syntax

Development Compiler
, ,
1

I’m working on adding support for parsing async deinit in swift-syntax, but I’m a bit stuck with producing diagnostics.

For which cases does it make sense to produce customised diagnostics?

These two probably are must haves:

// error: expected async specifier; did you mean 'async'?
// fixit: replace 'reasync' with 'async' 
deinit await

// error: deinit cannot throw
// fixit: remove 'throws'
deinit throws

Is one error sufficient here? Is second error useful in this case, or it does more harm than good?

// error: deinit cannot throw
// fixit: remove 'throws'
// error: 'async' must precede 'throws'
// fixit: move 'async' in front of 'throws'
deinit throws async

Should there be a diagnostics for async before deinit? To my surprise currently this parses without any diagnostics:

async func foo()
async deint

How should the parse tree like?

I’ve found existing code for parsing and producing diagnostics for effect specifiers for functions, accessors and types, and my first instinct was to generalise and reuse it for deinit case, but actually so far it's not working great.

I've created DeinitEffectSpecifiers node, which contains only AsyncSpecifier, without ThrowsSpecifier, and then in extension conforming RawDeinitEffectSpecifiersSyntax to RawEffectSpecifiersTrait added convenience initialiser that combines unexpectedBetweenAsyncSpecifierAndThrowsSpecifier, throwsSpecifier and unexpectedAfterThrowsSpecifier into unexpectedAfterAsyncSpecifier. MisspelledThrowsTokenKinds contains all 4 keywords, and CorrectThrowsTokenKinds is uninhabited. With minimal changes I was able to reuse parseEffectSpecifiers(), but diagnostics does not work well in this case.

In particular when trying to parse deinit throws async, I end up with asyncSpecifier being a synthesised token, while both real tokens end up in unexpectedAfterAsyncSpecifier.

I was hoping to reuse logic for producing AsyncMustPrecedeThrows, but exchangeTokens() expect a single unexpected token, but I have two in this case.

I might be able to fix it by handling throws and other keywords as MisspelledAsyncSpecifiers, but that feels too hacky.

So, I'm considering the following options:

  • place AsyncSpecifier directly into DeinitializerDecl.
  • create DeinitEffectSpecifiers without ThrowsSpecifier .
  • create DeinitEffectSpecifiers with ThrowsSpecifier.
2 Likes
2

After some experimenting I came up with the following:

  1. Using DeinitEffectSpecifiers with a single asyncSpecifier to separate unexpected deinit name and params from effect specifiers error.

  2. Not reusing parseEffectSpecifiers() and RawEffectSpecifiersTrait, because parseEffectSpecifiers() consumes deinit name as unexpected token.

  3. When parsing entire function signature as a recovery strategy, I need to convert RawFunctionEffectSpecifiersSyntax to RawDeinitEffectSpecifiersSyntax. Code responsible for that handles inserted missing async and attempts to find the real one after throws. I might be able to avoid this conversion, if I reuse only parseParameterClause() and drop consuming arrow and output type as unexpected tokens.

  4. I'm producing diagnostics when async is misspelled as await or reasync or when it is duplicated.

  5. I'm producing a single diagnostic for all occurrences of the throw-like keywords in the deinit effect specifiers, and do not issue separate diagnostic if async and throws were swapped.

  6. To be able to handle case of deinit reasync throws {} I have to loosen the logic in exchangeTokens() and allow exchange if there are unrelated tokens. I don't fully understand why this restriction was there in the first place, so I'm a bit concerned about this change. But existing tests are passing.

3

PR - Support for isolated and async deinit by nickolas-pohilets · Pull Request #1656 · apple/swift-syntax · GitHub