Skip to content

Prune impossible types from exhaustivity checker #3574

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
merged 2 commits into from
Dec 14, 2017

Conversation

OlivierBlanvillain
Copy link
Contributor

The exhaustivity checker sometimes looks for impossible types.

The test case added by this PR is an example taken from the shapeless test suite where a sealed trait is extended in two different scopes. The added case classesOne and Two can only appear withing object Atoms01 and Atoms02 respectively. On master the exhaustivity checker reports missing cases of the following form: PathVariantDefns.Atom2 & PathVariantDefns.Atoms01.type(PathVariantDefns.Atoms01).Two(_). Given that PathVariantDefns.Atoms01.type is the type of an object, the intersection between this type and any unrelated type is always empty. This PR adds a check to instantiate that detect and remove these cases.

@liufengyun WDYT?

NoType
} else {
result
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

In the check, there's a facility named implementability from a contributor which does more or less the same thing. What do you think we can generalise/adapt implementability to handle the case?

In particular, I think it's good to rename it to def inhabited(tp: Type): Boolean, and it should be able to handle the current case as well. WDYT?

Copy link
Contributor Author

@OlivierBlanvillain OlivierBlanvillain Nov 28, 2017

Choose a reason for hiding this comment

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

I think implementability computes if a type can be implemented by other types (singleton types can't!), while inhabited would need to check if there could actually exists an instance of that type, so I'm not sure two logics can be merged.

@AleksanderBG what's your opinion on that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think implementability should be refactored to have the semantics of inhabited. According to the core algorithm of exhaustivity check, it should only depend on the semantics of def inhabited(tp: Type): Boolean.

Copy link
Contributor

Choose a reason for hiding this comment

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

@OlivierBlanvillain I believe @liufengyun is correct - the function can be safely changed to check if a type is inhabited instead. This won't change the result for and-types, which is the only result intersectUnrelatedAtomicTypes should care about. I considered a similar refactoring myself when working on #3454. With the benefit of hindsight, type inhabitation is a far more generic notion and I should've tried to implement it from the beginning.

Copy link
Contributor

@liufengyun liufengyun left a comment

Choose a reason for hiding this comment

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

LGTM, we can do the refactoring of implementability in another PR.

@liufengyun liufengyun merged commit 406a064 into scala:master Dec 14, 2017
@liufengyun liufengyun deleted the path-variant-defns branch December 14, 2017 14:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants