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

Detect when resolve needs ModuleRef when likely cyclic references #3878

Merged
merged 18 commits into from
Nov 1, 2024

Conversation

myyk
Copy link
Contributor

@myyk myyk commented Oct 31, 2024

This completes #3715 with all the cycle cases I could think of.

It also touches resolveDirectChildren where I made some changes that I would help with readability. I thought I was going to need to build on those but didn't. These can be reverted if it's unwanted.

@myyk myyk changed the title Resolve need moduleref Detect when resolve needs ModuleRef when likely cyclic references Oct 31, 2024
@myyk
Copy link
Contributor Author

myyk commented Nov 1, 2024

I added some cycle detection for the direct dependencies but I'm stuck on cases when it's foo.__.bar-shaped where foo or bar can be anything, even _.

You can run the tests with ./mill main.resolve.test:

[960-2] ----------------------------------- Failures -----------------------------------
[960-2] X mill.resolve.ResolveTests.nested.wildcard3 3ms 
[960-2]   utest.AssertionError: resolvedTasks.map(_.map(_.toString).toSet[String]) ==
[960-2]   resolvedTasks: Either[String,List[mill.define.NamedTask[Any]]] = Left(Cyclic module reference detected: [class mill.util.TestGraphs$nestedModule$classInstance$], it's required to wrap it in ModuleRef.
[960-2]   Cyclic module reference detected: [class mill.util.TestGraphs$nestedModule$nested$], it's required to wrap it in ModuleRef.)
[960-2]   expected: scala.util.Either[String,scala.collection.immutable.Set[mill.define.NamedTask[Any]]] = Right(Set(classInstance.single, nested.single))
[960-2]     utest.asserts.Asserts$.assertImpl(Asserts.scala:30)
[960-2]     mill.resolve.ResolveTests$Checker.checkSeq(ResolveTests.scala:25)
[960-2]     mill.resolve.ResolveTests$Checker.apply(ResolveTests.scala:15)
[960-2]     mill.resolve.ResolveTests$.$anonfun$tests$66(ResolveTests.scala:230)
[960-2] X mill.resolve.ResolveTests.cross.double.wildcard.labelNeg4 3ms 
[960-2]   utest.AssertionError: resolvedTasks.map(_.map(_.toString).toSet[String]) ==
[960-2]   resolvedTasks: Either[String,List[mill.define.NamedTask[Any]]] = Left(Cyclic module reference detected: [class mill.util.TestGraphs$doubleCross$cross$], it's required to wrap it in ModuleRef.)
[960-2]   expected: scala.util.Either[String,scala.collection.immutable.Set[mill.define.NamedTask[Any]]] = Left(Cannot resolve cross.__.doesntExist. Try `mill resolve cross._` to see what's available.)
[960-2]     utest.asserts.Asserts$.assertImpl(Asserts.scala:30)
[960-2]     mill.resolve.ResolveTests$Checker.checkSeq(ResolveTests.scala:25)
[960-2]     mill.resolve.ResolveTests$Checker.apply(ResolveTests.scala:15)
[960-2]     mill.resolve.ResolveTests$.$anonfun$tests$113(ResolveTests.scala:365)
[960-2] Tests: 161, Passed: 159, Failed: 2

It's showing these 2 false positives because those are not cycles. These are the only noncyclic foo.__.bar test cases in there and I think that's the commonality.

@myyk
Copy link
Contributor Author

myyk commented Nov 1, 2024

The cleanup looks good to me

@lihaoyi lihaoyi merged commit dc0f678 into com-lihaoyi:main Nov 1, 2024
24 checks passed
@lihaoyi
Copy link
Member

lihaoyi commented Nov 1, 2024

Thanks @myyk! I'll close out the bounty over whatsapp

@lefou lefou added this to the 0.12.2 milestone Nov 1, 2024
@myyk myyk deleted the resolve-need-moduleref branch November 2, 2024 11:00
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.

3 participants