Skip to content

[QoI] Fail to propagateLValueAccessKind only if it's different from pre-existing #6433

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

Closed
wants to merge 1 commit into from

Conversation

xedin
Copy link
Contributor

@xedin xedin commented Dec 21, 2016

Change assertion from checking only for presence of LValue access
kind to check if the access kind stays the same. This is required
because CodeCompletionCallback code is going to attempt to
re-typecheck the whole module when there are delayed functions
present, so instead of failing if the access kind is set let's fail
only if it's set to a different value.

…re-existing

Change assertion from checking only for presence of LValue access
kind to check if the access kind stays the same. This is required
because CodeCompletionCallback code is going to attempt to
re-typecheck the whole module when there are delayed functions
present, so instead of failing if the access kind is set let's fail
only if it's set to a different value.
@xedin
Copy link
Contributor Author

xedin commented Dec 21, 2016

@slavapestov This is the only sane way I found to fix the problem - relaxing existing assertion, I've double checked correctness of the CodeCompletionCallbackImpl::typecheckContextImpl, it's a valid to try to re-typecheck the parent context all the way to the module level since there is no way to know what is type checked and what is not...

@slavapestov
Copy link
Contributor

I think this PR is hinting at the fact that re-typechecking is considered undesirable: #4605

Indeed I can think of several cases where typechecking the same Expr twice won't work. I just fixed a similar problem with 'lazy' properties that I'll push soon.

@benlangmuir Any thoughts?

@benlangmuir
Copy link
Contributor

Yes, right now there is no way to detect these double-typechecking situations reliably before they happen in code-completion (e.g. it's not sufficient to strip the types off the expression, as this patch will evidence). In the past we've jumped through hoops to avoid these situations from happening. I don't think relaxing this assertion is the right long-term answer here unless we agree that type-checking an expression again should be allowed.

@xedin
Copy link
Contributor Author

xedin commented Dec 21, 2016

@slavapestov @benlangmuir Sounds good, like like #4605 is going to be a proper solution for the problem, so i'm going to close this one.

@xedin xedin closed this Dec 21, 2016
@xedin
Copy link
Contributor Author

xedin commented Dec 21, 2016

@slavapestov btw, since you mentioned it I was working on the problem with lazy properties which i think might be related to double type-checking if not exactly the same, here is an example of it:

protocol P {
   func foo()
}

struct S {
  lazy var bar = P.foo
}

This is going to produce

Already type-checked
UNREACHABLE executed at /Users/xedin/work/c++/swift-env/swift/lib/Sema/CSGen.cpp:2686!
0  swift                    0x000000010f043915 llvm::sys::PrintStackTrace(llvm::raw_ostream&) + 101
1  swift                    0x000000010f043f89 PrintStackTraceSignalHandler(void*) + 25
2  swift                    0x000000010f03fb89 llvm::sys::RunSignalHandlers() + 425
3  swift                    0x000000010f044492 SignalHandler(int) + 354
4  libsystem_platform.dylib 0x00007fffb46b9bba _sigtramp + 26

Please let me know if that's something that you changes are going to fix so I can move on :)

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.

3 participants