Skip to content

[AST][Sema] Fixes the IterativeTypeChecker and better manages circular protocol inheritance #2123

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[AST][Sema] Fixes the IterativeTypeChecker and better manages circular protocol inheritance #2123

wants to merge 1 commit into from

Conversation

manavgabhawala
Copy link
Contributor

What's in this pull request?

Improves the diagnostics produced for circular protocol inheritance, further it improves the IterativeTypeChecker to use loops instead of recursion to help minimize the stack size and lastly improves the efficiency of detecting circular inheritance by making it a part of the main cycle when checking for inherited protocols.

Before merging this pull request to apple/swift repository:

  • Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform Comment
All supported platforms @swift-ci Please smoke test
OS X platform @swift-ci Please smoke test OS X platform
Linux platform @swift-ci Please smoke test Linux platform

Validation Testing

Platform Comment
All supported platforms @swift-ci Please test
OS X platform @swift-ci Please test OS X platform
Linux platform @swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

@tkremenek
Copy link
Member

@swift-ci test

@tkremenek
Copy link
Member

@jopamer Please review.

@tkremenek
Copy link
Member

@swift-ci test

@manavgabhawala manavgabhawala force-pushed the circular-protocol-inheritance branch 3 times, most recently from cc7ba2c to 92b7ccb Compare April 13, 2016 01:44
@jopamer
Copy link
Contributor

jopamer commented Apr 13, 2016

LGTM.

@manavgabhawala manavgabhawala force-pushed the circular-protocol-inheritance branch from 92b7ccb to 4295970 Compare April 15, 2016 01:13
@slavapestov
Copy link
Contributor

@swift-ci Please test

@slavapestov
Copy link
Contributor

Awesome work @manavgabhawala, it's really great to see the iterative decl checker getting some attention.

@manavgabhawala
Copy link
Contributor Author

The linux build timed out, and I'm not really sure why SourceKit/DocSupport/doc_clang_module.swift and SourceKit/DocSupport/doc_swift_module.swift. Any help to figure out how I can fix them would be great.

@DougGregor
Copy link
Member

@swift-ci Please test and merge

@manavgabhawala
Copy link
Contributor Author

@DougGregor I think the builds timed out. Is there something I need to do to fix it?

@CodaFi
Copy link
Contributor

CodaFi commented Jun 11, 2016

@harlanhaskins @DougGregor @jopamer Ping.

This patch needs to be retested.

@harlanhaskins
Copy link
Contributor

@swift-ci please test

@harlanhaskins
Copy link
Contributor

@shahmishal This build keeps timing out, and I have no idea why...

@CodaFi
Copy link
Contributor

CodaFi commented Jun 11, 2016 •
edited
Loading

Perhaps a rebase would do it (it is quite old). @manavgabhawala Can you git pull --rebase upstream master?

@manavgabhawala manavgabhawala force-pushed the circular-protocol-inheritance branch from 4295970 to 4da9267 Compare June 11, 2016 06:04
@manavgabhawala
Copy link
Contributor Author

@CodaFi done

@harlanhaskins
Copy link
Contributor

@swift-ci please test this pull request

@harlanhaskins
Copy link
Contributor

@swift-ci please test

@manavgabhawala
Copy link
Contributor Author

@harlanhaskins any update on why its timing out?

@shahmishal
Copy link
Member

@swift-ci please test

slavapestov
slavapestov reviewed Jun 23, 2016
View reviewed changes
lib/AST/ConformanceLookupTable.cpp
@@ -458,7 +458,9 @@ bool ConformanceLookupTable::addProtocol(NominalTypeDecl *nominal,

case ConformanceEntryKind::Implied:
// An implied conformance is better than a synthesized one.
if (kind == ConformanceEntryKind::Synthesized)
// Ignore implied ciruclar protocol inheritance
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo -- "ciruclar"

@manavgabhawala
[AST][Sema] Fixes the IterativeTypeChecker and better manages circula…
6b60db3 
…r protocol inheritance

The IterativeTypeChecker now use loops instead of recursion to help keep the stack size low
We diagnose circular dependencies for protocols in a more efficient manner and also prevent the possibility of infinite loops
@manavgabhawala manavgabhawala force-pushed the circular-protocol-inheritance branch from 4da9267 to 6b60db3 Compare June 23, 2016 07:49
@slavapestov
Copy link
Contributor

@swift-ci Please test and merge

@rintaro
Copy link
Member

rintaro commented Jul 25, 2016

Tested locally,
Just reverting changes in IterativeTypeChecker.cpp solves the problem.

$ git fetch origin pull/2123/head:PR-2123
$ git checkout PR-2123
$ git rebase master

# Resolve a conflict (caused by `defer` => `SWIFT_DEFER` naming change)
# in IterativeTypeChecker.cpp

$ utils/build-script -RT

# Confirmed it never finish
# validation-test/compiler_crashers/28261-swift-iterativetypechecker-satisfy.swift
# validation-test/SIL/crashers/032-swift-iterativetypechecker-satisfy.sil
^C

$ git checkout master -- lib/Sema/IterativeTypeChecker.cpp
$ utils/build-script -RT

<success>

$

@CodaFi
Copy link
Contributor

CodaFi commented Jul 25, 2016

@rintaro I can shepherd these changes in with another pull request.

@CodaFi CodaFi mentioned this pull request Jul 25, 2016
Merged
1 task
@CodaFi
Copy link
Contributor

CodaFi commented Jul 26, 2016

Superseded by the merge of #3747. Thanks @manavgabhawala!

@CodaFi CodaFi closed this Jul 26, 2016
@manavgabhawala manavgabhawala deleted the circular-protocol-inheritance branch July 30, 2016 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jopamer jopamer

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@manavgabhawala @tkremenek @jopamer @slavapestov @DougGregor @CodaFi @harlanhaskins @shahmishal @rintaro