Skip to content

Make sure ErrorTypes containing type variables are marked as such #7967

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

Conversation

jrose-apple
Copy link
Contributor

  • Explanation: To make a long story short, we managed to make long-lived types refer to short-lived types; if a new type was reallocated at the exact same address, the original cached long-lived type would be considered "the same" and reused, even though it had the wrong settings. This led to nonsensical errors like 'C.Iterator' is not the same type as 'C.Iterator'. More detail in the commit message.
  • Scope: Unknown. We haven't identified any failures on the 3.1 branch as being a result of this bug, but it wouldn't necessarily be obviously this issue. Even on master it appeared only every hundred runs or so.
  • Issue: rdar://problem/30382791
  • Reviewed by: @DougGregor
  • Risk: Low. The fix marks some more types as "short-lived" by noting that they have type variables inside them. The lifetime change should have no effect, but there's a very small chance that something else in the compiler will not be able to handle the "has type variables" flag being set here. We think it's unlikely, though.
  • Testing: Ran the previously failing compile command 400 times without observing a failure (on the master branch).

…wiftlang#7963)

In some cases, the type checker will produce error types with the
"original type" intact for recovery purposes. Like other types, when
the original type contains a type variable, the ErrorType instance
will be allocated in the "temporary" memory arena associated with the
active constraint solver, because there's no way that particular error
will come up again once the constraint system containing that type
variable has been destroyed.

However, we weren't propagating that "contains a type variable"
information to the newly-created ErrorType, which meant that any type
/containing/ that ErrorType would be allocated in the "permanent"
arena. In practice, this would always be a DependentMemberType; not
too many types are created without looking at their base types at all.
The arena containing the ErrorType would then be deallocated, and its
memory reused later on for a /different/ type. If we ever tried to
make a DependentMemberType whose base was this new type, we'd find the
old DependentMemberType instance in our cache and return that. The
result was that we'd have a DependentMemberType whose "HasError" bit
was set even though the base type was not an error type, and which was
considered canonical whether or not the base type was. This would then
either hit an assertion later on or result in nonsensical errors like
"'C.Iterator' is not the same type as 'C.Iterator'".

Because the reused address always referred to a valid type, none of
the usual dynamic analysis tools could catch the problem. It really
comes down to using a pointer address as a key in a map---but even
without that, we were allocating types in the permanent arena that
really should be temporary, which is a waste of memory.

Likely fixes rdar://problem/30382791, a nondeterministic failure we've
been seeing for weeks on the bots and on developer machines.
@jrose-apple jrose-apple added this to the Swift 3.1 milestone Mar 7, 2017
@jrose-apple
Copy link
Contributor Author

@swift-ci Please test

@tkremenek tkremenek merged commit 00b540b into swiftlang:swift-3.1-branch Mar 8, 2017
@jrose-apple jrose-apple deleted the 3.1-to-err-is-human branch March 8, 2017 18:06
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.

2 participants