Skip to content

[Diagnostics] Transfer previously resolved types directly from expressions #17947

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 3 commits into from
Jul 14, 2018

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Jul 13, 2018

While trying to diagnose the problem with previously type-checked
sub-expression, use its type-checked variant as a source of type
information, instead of transferring from its original constraint system,
because if expression was type-checked successfully it would
have all of the required information in AST, and that doesn't
rely on associated constraint system being present.

Resolves: rdar://problem/42056741

…sions

While trying to diagnose the problem with previously type-checked
sub-expression, use its type-checked variant as a source of type
information, instead of transferring from its original constraint system,
because if expression was type-checked successfully it would
have all of the required information in AST, and that doesn't
rely on associated constraint system being present.

Resolves: rdar://problem/42056741
@xedin
Copy link
Contributor Author

xedin commented Jul 13, 2018

@swift-ci please test

@xedin xedin requested a review from rudkx July 14, 2018 00:41
Copy link
Contributor

@rudkx rudkx left a comment

Choose a reason for hiding this comment

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

LGTM. That code was required at one point but I suspect we're caching more types in enough places that it's now more of a problem than a solution.

You can remove the tranferExprTypes() method at this point, too.

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test source compatibility

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

Removing transferExprTypes in the separate commit so it could be reverted if needed.

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test source compatibility

@slavapestov
Copy link
Contributor

Can you add the type checker test case as well? IIRC it didn’t crash on master but still emitted a duplicate diagnostic.

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

The example I've added doesn't crash without code-completion because that triggers diagnostics differently, I couldn't make it crash without using code completion, and it also doesn't produce duplicate diagnostics either, you might be thinking of a different example...

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 11e1d13

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

3 LLDB tests are failing again:

23:22:02 FAIL: test_swift_conditional_breakpoint_dwarf (lang/swift/conditional_breakpoints/TestSwiftConditionalBreakpoint.py)
23:22:02 FAIL: test_swift_stepping_dwarf (lang/swift/stepping/TestSwiftStepping.py)
23:22:02 FAIL: test_swift_struct_init_dwarf (lang/swift/struct_init_display/TestSwiftStructInit.py)

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test Linux platform

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test source compatibility

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 939f6d4

@xedin
Copy link
Contributor Author

xedin commented Jul 14, 2018

@swift-ci please test macOS platform

@xedin xedin merged commit 45634bd into swiftlang:master Jul 14, 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.

4 participants