Navigation Menu

Skip to content
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

Conversation

DougGregor
Copy link
Member

@DougGregor 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 [GSB] Improve handling of concrete types, type aliases, recursion [4.0] [GSB] Improve handling of concrete types, type aliases, recursion Jun 24, 2017
@DougGregor
Copy link
Member Author

@swift-ci please test

@DougGregor
Copy link
Member Author

@swift-ci please test compatibility

@DougGregor
Copy link
Member Author

@swift-ci please test compatibility suite

@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

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
Copy link
Member Author

@swift-ci please test source compatibility

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please test source compatibility

@DougGregor
Copy link
Member Author

@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;
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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>
Copy link
Member

Choose a reason for hiding this comment

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

Why did you change the crasher test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member

LGTM!

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)
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)
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)
…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)
…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)
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)
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)
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)
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)
…ass.

Fixes one recently-found crasher.

(cherry picked from commit e256a9d)
Fixes two more recently-found GSB crashers.
@DougGregor DougGregor force-pushed the gsb-concrete-types-recursion branch from 24e8975 to 19f4ee7 Compare June 26, 2017 18:11
@DougGregor
Copy link
Member Author

@swift-ci please test

1 similar comment
@DougGregor
Copy link
Member Author

@swift-ci please test

@tkremenek tkremenek merged commit 1c34375 into apple:swift-4.0-branch Jun 26, 2017
@DougGregor DougGregor deleted the gsb-concrete-types-recursion branch June 26, 2017 22:32
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.

None yet

3 participants