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

Fix #21721: make case TypeBlock(_, _) not match non-type Block #21722

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

felher
Copy link
Contributor

@felher felher commented Oct 7, 2024

TypeBlocks are represented as normal Blocks in the Quotes API's implementation. The current TypeTest for TypeBlock is exactly the same as the TypeTest for Block, which means that case TypeBlock(_, _) "matches" every block.

The implementation of unapply on TypeBlockModule, however, gives back (List[TypeDef], TypeTree). It constructs the List[TypeDef] by mapping over every statement of the block, trying to turn it into a TypeDef by using a match with the pattern

  case alias: TypeDef => alias

This seems fine since TypeBlocks are supposed to be just a list of TypeDefs followed by a type as the last expression. Since the TypeTest matches any Block and not only Blocks that are TypeBlocks, the statements can be anything, not just TypeDefs, which lets the whole case TypeBlock(_, _) pattern die with a MatchError.

This commit fixes the problem by making the TypeTest check whether the Block is a type (which in turns checks whether the Blocks expression is a type)

This is my first PR, please let me know if I should change anything or am completely going about fixing this the wrong way, etc.

Also, I could not run the complete test suite on my machine, it died even without making any changes. But I could at least run the positive compilation tests.

Closes #21721

TypeBlocks are represented as normal Blocks in the Quotes API. The
current TypeTest for TypeBlock is exactly the same as the TypeTest for
Block, which means that case TypeBlock(_, _) matches every block.

The implementation of unapply on TypeBlockModule, however, gives back
(List[TypeDef], TypeTree). It constructs the List[TypeDef] by mapping
over every statement, turning it into a TypeDef by using a match
with the pattern

  case alias: TypeDef => alias

Since the TypeTest matches any Block and not only Blocks that are
TypeBlocks, the statemnts can be anything, not just TypeDefs, which lets
the whole case TypeBlock(_, _) pattern fail with a MatchError.

This commit fixes the problem by making the TypeTest check whether the
Block is a type (which in turns checks whether the blocks expression is
a type)
@felher
Copy link
Contributor Author

felher commented Oct 7, 2024

I'm a bit unsure whether the failure of the CI has anything to do with the change proposed in this PR.

On my machine, scala3-bootstrapped/testOnly dotty.tools.dotc.CompilationTests runs through fine. No problem like

Error:  Test dotty.tools.dotc.CompilationTests.runAll failed: java.lang.AssertionError: Run test failed, but should not, reasons:
Error:  encountered 1 test failure(s):
Error:    - test 'tests\run\lambda-serialization-gc.scala' timed out, took 179.28 sec
Error:      at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkPass(ParallelTesting.scala:1257)
Error:      at dotty.tools.vulpix.ParallelTesting$CompilationTest.checkRuns(ParallelTesting.scala:1221)
Error:      at dotty.tools.dotc.CompilationTests.runAll(CompilationTests.scala:168)
Error:      ...

I got:

[info] Test dotty.tools.dotc.CompilationTests.runAll started
[=======================================>] completed (1806/1806, 0 failed, 94s)

@hamzaremmal
Copy link
Member

Hi @felher, the CI failure has nothing to do with the proposed change. I've restarted the failed job.

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.

TypeTest of TypeBlock of Quotes API matches every Block
2 participants