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

[4.0] [GSB] Improve handling of concrete types, type aliases, recursion #10565

Merged
merged 17 commits into from Jun 26, 2017

Conversation

Projects
None yet
3 participants
@DougGregor
Member

DougGregor commented Jun 24, 2017

Explanation: Improves our handling of concrete conformances, type aliases in protocols, and same-type-to-concrete constraints in the generic signature builder to make them all more consistent and robust.
Scope: Affects many uses of generic code; will reject a number of ill-formed cases that would previously crash the compiler.
Radar: SR-4295 / rdar://problem/31372308, SR-4757 / rdar://problem/31912838, SR-4786 / rdar://problem/31955862, SR-5014 / rdar://problem/32402482, SR-4737 / rdar://problem/31905232, plus a few other dupes.
Risk: Low-ish. The generic signature builder is key to all generics handling in Swift, so changes there can cause breakage... but it's also fairly well-tested via the standard library, so it's likely we'd have caught regressions.
Testing: Compiler regression testing, plus lots of new tests from the various fixed radars.

@DougGregor DougGregor changed the title from [GSB] Improve handling of concrete types, type aliases, recursion to [4.0] [GSB] Improve handling of concrete types, type aliases, recursion Jun 24, 2017

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test

Member

DougGregor commented Jun 24, 2017

@swift-ci please test

@DougGregor DougGregor requested review from huonw and slavapestov Jun 24, 2017

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test compatibility

Member

DougGregor commented Jun 24, 2017

@swift-ci please test compatibility

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test compatibility suite

Member

DougGregor commented Jun 24, 2017

@swift-ci please test compatibility suite

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test source compatibility

Member

DougGregor commented Jun 24, 2017

@swift-ci please test source compatibility

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

The source-compatibility suite died with an exception:

FATAL: command execution failed
java.io.EOFException
at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2335)
at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:2804)
at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:802)
at java.io.ObjectInputStream.(ObjectInputStream.java:299)
at hudson.remoting.ObjectInputStreamEx.(ObjectInputStreamEx.java:48)
at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:34)
at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:59)
Caused: java.io.IOException: Unexpected termination of the channel
at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:73)
Caused: java.io.IOException: Backing channel 'macOS-37' is disconnected.
at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:192)
at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:257)
at com.sun.proxy.$Proxy91.isAlive(Unknown Source)
at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1043)
at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1035)
at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:155)
at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:109)
at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
at hudson.model.Build$BuildExecution.build(Build.java:206)
at hudson.model.Build$BuildExecution.doRun(Build.java:163)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:534)
at hudson.model.Run.execute(Run.java:1728)
at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
at hudson.model.ResourceController.execute(ResourceController.java:98)
at hudson.model.Executor.run(Executor.java:405)
Build step 'Execute shell' marked build as failure
ERROR: Step ‘Archive the artifacts’ failed: no workspace for swift-PR-source-compat-suite #247

Member

DougGregor commented Jun 24, 2017

The source-compatibility suite died with an exception:

FATAL: command execution failed
java.io.EOFException
at java.io.ObjectInputStream$PeekInputStream.readFully(ObjectInputStream.java:2335)
at java.io.ObjectInputStream$BlockDataInputStream.readShort(ObjectInputStream.java:2804)
at java.io.ObjectInputStream.readStreamHeader(ObjectInputStream.java:802)
at java.io.ObjectInputStream.(ObjectInputStream.java:299)
at hudson.remoting.ObjectInputStreamEx.(ObjectInputStreamEx.java:48)
at hudson.remoting.AbstractSynchronousByteArrayCommandTransport.read(AbstractSynchronousByteArrayCommandTransport.java:34)
at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:59)
Caused: java.io.IOException: Unexpected termination of the channel
at hudson.remoting.SynchronousCommandTransport$ReaderThread.run(SynchronousCommandTransport.java:73)
Caused: java.io.IOException: Backing channel 'macOS-37' is disconnected.
at hudson.remoting.RemoteInvocationHandler.channelOrFail(RemoteInvocationHandler.java:192)
at hudson.remoting.RemoteInvocationHandler.invoke(RemoteInvocationHandler.java:257)
at com.sun.proxy.$Proxy91.isAlive(Unknown Source)
at hudson.Launcher$RemoteLauncher$ProcImpl.isAlive(Launcher.java:1043)
at hudson.Launcher$RemoteLauncher$ProcImpl.join(Launcher.java:1035)
at hudson.tasks.CommandInterpreter.join(CommandInterpreter.java:155)
at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:109)
at hudson.tasks.CommandInterpreter.perform(CommandInterpreter.java:66)
at hudson.tasks.BuildStepMonitor$1.perform(BuildStepMonitor.java:20)
at hudson.model.AbstractBuild$AbstractBuildExecution.perform(AbstractBuild.java:779)
at hudson.model.Build$BuildExecution.build(Build.java:206)
at hudson.model.Build$BuildExecution.doRun(Build.java:163)
at hudson.model.AbstractBuild$AbstractBuildExecution.run(AbstractBuild.java:534)
at hudson.model.Run.execute(Run.java:1728)
at hudson.model.FreeStyleBuild.run(FreeStyleBuild.java:43)
at hudson.model.ResourceController.execute(ResourceController.java:98)
at hudson.model.Executor.run(Executor.java:405)
Build step 'Execute shell' marked build as failure
ERROR: Step ‘Archive the artifacts’ failed: no workspace for swift-PR-source-compat-suite #247

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test source compatibility

Member

DougGregor commented Jun 24, 2017

@swift-ci please test source compatibility

1 similar comment
@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 24, 2017

Member

@swift-ci please test source compatibility

Member

DougGregor commented Jun 24, 2017

@swift-ci please test source compatibility

@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 26, 2017

Member

@huonw , you reviewed the first 6 of these commits on master

Member

DougGregor commented Jun 26, 2017

@huonw , you reviewed the first 6 of these commits on master

@@ -826,7 +835,7 @@ class GenericSignatureBuilder::RequirementSource final
TypeBase *type;
/// A protocol conformance used to satisfy the requirement.
ProtocolConformance *conformance;
void *conformance;

This comment has been minimized.

@slavapestov

slavapestov Jun 26, 2017

Member

Why can't this just be a ProtocolConfomanceRef? You're including the header above

@slavapestov

slavapestov Jun 26, 2017

Member

Why can't this just be a ProtocolConfomanceRef? You're including the header above

This comment has been minimized.

@DougGregor

DougGregor Jun 26, 2017

Member

sigh. Non-POD types are really annoying to work with in unions.

@DougGregor

DougGregor Jun 26, 2017

Member

sigh. Non-POD types are really annoying to work with in unions.

@@ -7,7 +7,7 @@ protocol MySequenceType {}
protocol MyIndexableType {}
protocol MyCollectionType : MySequenceType, MyIndexableType {
typealias SubSequence = MySlice<Self>
associatedtype SubSequence = MySlice<Self>

This comment has been minimized.

@slavapestov

slavapestov Jun 26, 2017

Member

Why did you change the crasher test case?

@slavapestov

slavapestov Jun 26, 2017

Member

Why did you change the crasher test case?

This comment has been minimized.

@DougGregor

DougGregor Jun 26, 2017

Member

The test case itself was ill-formed because of clashing typealias definition, but we didn't catch it before because the GSB was ignoring the inconsistency. I believe the intent here was to use associatedtype with defaults, because the test itself was intended to be well-formed and go all the way to IRGen. This change makes the test case compile again.

@DougGregor

DougGregor Jun 26, 2017

Member

The test case itself was ill-formed because of clashing typealias definition, but we didn't catch it before because the GSB was ignoring the inconsistency. I believe the intent here was to use associatedtype with defaults, because the test itself was intended to be well-formed and go all the way to IRGen. This change makes the test case compile again.

@slavapestov

This comment has been minimized.

Show comment
Hide comment
@slavapestov

slavapestov Jun 26, 2017

Member

LGTM!

Member

slavapestov commented Jun 26, 2017

LGTM!

DougGregor added some commits Jun 2, 2017

[GSB] Eliminate PotentialArchetype::NestedTypeUpdate.
NestedTypeUpdate was mostly just the internal name for
ArchetypeResolutionKind, but the translation was a bit lossy and there
was no point in having separate enums. Standardize on
ArchetypeResolutionKind, adding a new case (WellFormed) to capture the
idea that we can create a new potential archetype only when we know
there is a nested type with that name---and avoid creating unresolved
potential archetypes.

(cherry picked from commit fafeec0)
[GSB] Separate out a "structurally derived" requirement source kind.
Rather than abusing the "superclass" requirement source with a null
protocol conformance, introduce a separate "structurally derived"
requirement source kind for structurally-derived requirements that
don't need any additional information, e.g., the class layout
requirement derived from a superclass requirement.

(cherry picked from commit ffea1b3)
[GSB] Improve handling of conformances resolved by concrete types.
Centralize and simplify the handling of conformance requirements
resolved by same-type-to-concrete requirements in a few ways:

* Always store a ProtocolConformanceRef in via-superclass and
  via-concrete requirement sources, so we never lose this information.

* When concretizing a nested type based on its parent, use the
  via-concrete conformance information rather than performing lookup
  again, simplifying this operation considerably and avoiding
  redundant lookups.

* When adding a conformance requirement to a potential archetype that
  is equivalent to a concrete type, attempt to find and record the
  conformance.

Fixes SR-4295 / rdar://problem/31372308.

(cherry picked from commit 52e52b5)
[GSB] Centralize diagnosis of concrete types and conformance requirem…
…ents.

Ensures that we don't admit invalid cases where the concrete type does
not conform to the required protocol.

(cherry picked from commit c879b95)
[GSB] Concretize nested types when merging two potential archetypes.
When two potential archetypes are merged and only one of them was a
concrete type beforehand, concretize the nested types in the
equivalence class of the non-concrete potential archetype. Otherwise,
we're liable to miss redundancies.

This feels like an ad hoc extension to an ad hoc mechanism, but gets
us back to parity with this patch series.

(cherry picked from commit bf730ff)
[GSB] Cope with typealiases within protocol hierarchies.
When we see two type(aliase)s with the same name in a protocol
hierarchy, make them equal with an implied same-type requirement. This
detects inconstencies in typealiases across different protocols, and
eliminates the need for ad hoc consistency checking. This is a step
toward simplifying away the need for direct-diagnosis operations
involving concrete type mismatches.

While here, warn when we see an associated type with the same as a
typealias from an inherited protocol; in this case, the associated
type is basically useless, because it's going to be equivalent to the
typealias.

(cherry picked from commit c47aea7)
[GSB] When retrieving the archetype anchor, allow partial results.
Specifically, we need to be able to add a new potential archetype for
the anchor. This API might need refinement.

(cherry picked from commit aeb5b01)
[GSB] Avoid recursively growing increasingly-nested potential archety…
…pes.

In some circumstances, we could end up growing increasingly-nested
potential archetypes due to a poor choice of representatives and
anchors. Address this in two places:

* Always prefer to use the potential archetype with a lower nesting
  depth (== number of nested types) to one with a greater nesting
  depth, so we don't accumulate more nested types onto the
  already-longer potential archetypes, and

* Prefer archetype anchors with a lower nesting depth *except* that we
  always prefer archetype anchors comprised of a sequence of
  associated types (i.e., no concrete type declarations), which is
  important for canonicalization.

Fixes SR-4757 / rdar://problem/31912838, as well as a regression
involving infinitely-recursive potential archetypes caused by the
previous commit.

(cherry picked from commit a72a2bf)
[GSB] Don't add invalid concrete requirements.
When a concrete requirement is invalid due to the concrete type
lacking a conformance to a particular, required protocol, don't emit
that incorrect requirement---it causes invalid states further down the
line.

Fixes SR-5014 / rdar://problem/32402482.

While here, fix a comment that Huon noticed trailed off into oblivion.

(cherry picked from commit dd38697)

DougGregor added some commits Jun 26, 2017

[GSB] Ensure that we have superclass constraints from merged equiv cl…
…ass.

Fixes one recently-found crasher.

(cherry picked from commit e256a9d)
[GSB] Don't crash when substitution fails to produce a type.
Fixes two more recently-found GSB crashers.
@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 26, 2017

Member

@swift-ci please test

Member

DougGregor commented Jun 26, 2017

@swift-ci please test

1 similar comment
@DougGregor

This comment has been minimized.

Show comment
Hide comment
@DougGregor

DougGregor Jun 26, 2017

Member

@swift-ci please test

Member

DougGregor commented Jun 26, 2017

@swift-ci please test

@tkremenek tkremenek merged commit 1c34375 into apple:swift-4.0-branch Jun 26, 2017

4 checks passed

Swift Test Linux Platform 9976 tests run, 0 skipped, 0 failed.
Details
Swift Test Linux Platform (smoke test)
Details
Swift Test OS X Platform 40112 tests run, 0 skipped, 0 failed.
Details
Swift Test OS X Platform (smoke test)
Details

@DougGregor DougGregor deleted the DougGregor:gsb-concrete-types-recursion branch Jun 26, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment