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

Diagnose Circular Typealiases in Protocols #138

Merged
merged 2 commits into from
Dec 7, 2015

Conversation

CodaFi
Copy link
Contributor

@CodaFi CodaFi commented Dec 4, 2015

Fixes a longstanding issue where declarations of the form

protocol Foo {
    typealias X = X
}

would accidentally crash the compiler by sending the AST visitor into a loop with the typechecker.

The error type alias 'X' circularly references itself is now emitted at the declaration site.

@@ -573,6 +573,9 @@ class alignas(1 << DeclAlignInBits) Decl {
unsigned : NumTypeDeclBits;

unsigned Recursive : 1;

/// Whether or not this declaration is currently being type-checked.
unsigned BeingTypeChecked : 1;
};
enum { NumAssociatedTypeDeclBits = NumTypeDeclBits + 1 };
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be bumped from + 1 to + 2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cwillmor Great catch! Thanks.

@AlexDenisov
Copy link
Contributor

Hi @CodaFi, would be better if you could rebase against master. This way your patch will avoid merge-commits, that are not needed usually.

See this article for more details:
https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history

@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 4, 2015

@AlexDenisov I've been rebasing. Do you mean I should squash?

@AlexDenisov
Copy link
Contributor

@CodaFi I see there are few merge commits ('merge branch master ...'), besides that it could be one commit.

Do you mean I should squash?

the correct answer might be "rebase and squash" :)

@CodaFi CodaFi force-pushed the the-circular-typealias-of-life branch from de9cc72 to 214575b Compare December 4, 2015 18:06
@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 4, 2015

@AlexDenisov Ah, I see what was happening. Wrong script!

@AlexDenisov
Copy link
Contributor

@CodaFi thank you, not it looks better. You may think about such changes as of one atomic patch ;)

@swizzlr
Copy link

swizzlr commented Dec 5, 2015

It seems that the Swift team do not subscribe to the dogmatic view that rewriting history outside mainline is bad!

( 🎉 )

@CodaFi
Copy link
Contributor Author

CodaFi commented Dec 5, 2015

@DougGregor I believe this is ready for review when you're ready.

@CodaFi CodaFi force-pushed the the-circular-typealias-of-life branch from 214575b to 9b68f5f Compare December 5, 2015 21:05
@@ -3173,7 +3173,7 @@ class DeclChecker : public DeclVisitor<DeclChecker> {
TAD->getUnderlyingTypeLoc().setInvalidType(TC.Context);
} else if (TAD->getDeclContext()->isGenericContext()) {
TAD->setInterfaceType(
TC.getInterfaceTypeFromInternalType(TAD->getDeclContext(),
TC.getInterfaceTypeFromInternalType(TAD->getDeclContext(),
TAD->getType()));
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary whitespace change here.

Choose a reason for hiding this comment

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

I would go as far as saying that white space had meaning and now is gone. It was enforcing that getInterfaceTypeFromInternalType is an argument passed to setInterfaceType.
It's merged now but would you oppose adding this space back in? I'm not one for PR (just to correct this) with such minimal changes but...

DougGregor added a commit that referenced this pull request Dec 7, 2015
Diagnose Circular Typealiases in Protocols
@DougGregor DougGregor merged commit 44c46d6 into swiftlang:master Dec 7, 2015
jtbandes added a commit to jtbandes/swift that referenced this pull request Dec 7, 2015
This crashing test became non-crashing in swiftlang#138. swiftlang#138 also emitted multiple instances of the same diagnostic, which was causing -verify to fail on decl/protocol/req/recursion.swift.
dabelknap added a commit to dabelknap/swift that referenced this pull request Nov 19, 2018
Support throws/rethrows in function decls.
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
Added Toolchain section in INSTALL.md
slavapestov pushed a commit to slavapestov/swift that referenced this pull request Nov 27, 2018
Added Toolchain section in INSTALL.md

Signed-off-by: Daniel A. Steffen <dsteffen@apple.com>
@CodaFi CodaFi deleted the the-circular-typealias-of-life branch December 22, 2019 02:58
kateinoigakukun added a commit to kateinoigakukun/swift that referenced this pull request Feb 14, 2020
* Add swift_symbolgraph_extract variable for wasi to run lit test

* [WASM] Update WASI SDK to 0.2.0

When using wasm-ld in wasi-sdk 0.1.0-swiftwasm, some test cases in swift
are failed with "failed to demangle witness for associated type
'Iterator' in conformance 'Swift.UInt64.Words: Sequence' from mangled
name '$e'" message.

This message means that runtime library fails to look up for protocol
conformance from swift5_protocol_conformances section. In WebAssembly
binary, these protocol conformances data are stored in data segments in
each object files and link them into one segment by wasm-ld.

Swift runtime library uses special __start and __stop symbols to point
where the metadata section is located. These special symbols are emitted
by linker wasm-ld. But wasm-ld in wasi-sdk 0.1.0 emitited them for each
data segments without combining them by their name. This causes the
crash if an user code has a protocol conformance because __start and
__stop symbols point only the user defined protocol conformance
segment.

The wasm-ld bug has been fixed by this patch https://reviews.llvm.org/D64148
freak4pc pushed a commit to freak4pc/swift that referenced this pull request Sep 28, 2022
…vbar

Add Swift 4 commit for AMScrollingNavbar
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.

6 participants