Skip to content

[Sema] Don't ignore implicit AST nodes in diagnoseUnhandledThrowSite #61392

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

Merged
merged 2 commits into from
Oct 6, 2022

Conversation

theblixguy
Copy link
Collaborator

@theblixguy theblixguy commented Sep 30, 2022

Resolves #61368

This partially reverts d461eab as it caused a regression where the 5.7 compiler hangs (but crashes on main), because diagnoseUnhandledThrowSite was ignoring implicit nodes, which was causing a throwing property wrapper init call to not get diagnosed since it's created implicitly.

We used to diagnose it on 5.6, but stopped doing it from 5.7 onwards.

@theblixguy theblixguy requested a review from xedin September 30, 2022 22:18
@xedin
Copy link
Contributor

xedin commented Oct 3, 2022

@swift-ci please test

@theblixguy
Copy link
Collaborator Author

theblixguy commented Oct 3, 2022

Hmm, Concurrency/async_sequence_syntax.swift is failing.

<unknown>:0: error: unexpected error produced: call can throw, but the error is not handled
<unknown>:0: error: unexpected note produced: call is to 'rethrows' function, but a conformance has a throwing witness
<unknown>:0: error: diagnostic produced elsewhere: call can throw, but the error is not handled
<unknown>:0: note: diagnostic produced elsewhere: call is to 'rethrows' function, but a conformance has a throwing witness

for

@available(SwiftStdlib 5.1, *)
func missingThrows<T : AsyncSequence>(_ seq: T) async {
  for try await _ in seq { } // expected-error{{error is not handled because the enclosing function is not declared 'throws'}}
}

I guess we're creating implicit calls to the iterator which throws? So with this change we end up emitting these extra diagnostics, probably why they don't have source locations attached to it.

@xedin
Copy link
Contributor

xedin commented Oct 3, 2022

This might be the reason why I added that check, it might be possible to add source locations to synthesized code and get this diagnostics attached to the expression at least?

@theblixguy
Copy link
Collaborator Author

theblixguy commented Oct 5, 2022

@xedin Done! I added the source location of for to the synthesized next() call (reason I went with for is because that's where the throws diagnostic is attached to... although I don't think it really matters as long as it's somewhere on the for loop).

The diagnostics aren't great IMO because they actually refer to the synthesized calls, so we probably want them to be tailored to the for loop. It could be a good starter bug!

@xedin
Copy link
Contributor

xedin commented Oct 5, 2022

@swift-ci please test

@theblixguy theblixguy merged commit d124b35 into swiftlang:main Oct 6, 2022
@theblixguy theblixguy deleted the gh61368 branch October 6, 2022 10:25
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.

Compiler hangs compiling property wrapper instance with throwing default init
2 participants