-
-
Notifications
You must be signed in to change notification settings - Fork 359
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
Clean up Resolve.scala and related code to improve rigor and error reporting #2453
Merged
+1,585
−1,180
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
lihaoyi
changed the title
[WIP] Try to clean up Resolve.scala and related code
[WIP] Clean up Resolve.scala and related code to improve rigor and error reporting
Apr 25, 2023
lihaoyi
changed the title
[WIP] Clean up Resolve.scala and related code to improve rigor and error reporting
Clean up Resolve.scala and related code to improve rigor and error reporting
Apr 26, 2023
lefou
approved these changes
Apr 26, 2023
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a great change! 👏 I myself got lost multiple times in the resolver code. Looks good to me.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR overhauls
Resolve.scala
andResolve{Tasks,Metadata,Segments}.scala
, which is some of the oldest and most poorly organized code in the repo. This gives us several things:Improving maintainability and understandability: I can explain how the resolve logic works now, where I couldn't before. The way it called back and forth between
RunScripts
andResolve*
sub-classes and theResolve
superclass was crazyMake the resolve behavior more predictable: the previous implementation had a lot of copy-pasty code, both within
Resolve.scala
and betweenResolve{Tasks,Metadata,Segments}.scala
, with subtle changes between them especially in error reporting. e.g. the code to handle_
and__
was copy-pasted 4-5 times. Now the implementations share much more code, without weird copy-pasty divergences, and should behave much more consistentlyFix a lot of old bugs: e.g. when resolution failed, the error message sometimes (but not always) had the segments in the backwards order. Passing in a query to Mill with a parse error would sometimes (but not always) just truncate it at the parse error. Brace expansion would fail to expand empty sections e.g.
{,foo,bar}qux => qux fooqux barqux
. These are now fixed, along with many other issues mostly around edge cases or error reportingGreatly improve error reporting in the case of module initialization failure. These previously returned massive stacktraces from un-caught exceptions, and now return truncated short(ish) stacktraces with mostly the stack frames the user cares about it. The fact that the exceptions are caught also means we can properly unit test and integration test their contents
Improve the laziness of the module initialization process. Previously, we would initialize many more modules than we needed to while performing target resolution. Now we resolve a much tighter set:
build.sc
that takes a non-trivial amount of time to initialize all the modules, now you only initialize what you need to initialize depending on what command you are runningMajor Changes
RunScript.scala
,Resolve.scala
, andResolve{Tasks,Metadata,Segments}.scala
have been mostly rewritten. Now there are three main components:ResolveCore.scala
, that performs resolution of everything-that-can-be-resolved, wrapped inResolveCore.Resolved
objectsResolveNonEmpty.scala
, which wrapsResolveCore.scala
and standardizes the error reporting logic when resolution failsResolve.scala
, which containsResolveTasks
,ResolveMetadata
, andResolveSegments
that take theResolveCore.Resolved
objects and perform the minimal processing necessary for each use caseImproved the laziness of
ResolveCore.scala
.millModuleDirectChildren
that previously would force all children to be instantiated have been replaced by more targeted APIs, e.g..millModuleDirectChildren.collect { case b: RootModule => b }
is now.reflectNestedObjects[RootModule]()
,.millModuleDirectChildren.find(_.millOuterCtx.segment == Segment.Label(singleLabel))
is now.reflectNestedObjects0[Module](namePred: String => Boolean)
, etc.Module.scala
were refactored to allow reflecting on members without invoking them, or specifying the name of the thing you want to reflect on to select it directly rather than having to list out and instantiate everythingCross
modules also had some tweaks in order to returnMapView
s instead ofMap
s, to allow instantiation of the individualCross.Module
instances to happen separately and on-demandGreatly improved the strictness of error reporting of
ResolveCore.scala
. In particular, the places which can throw exceptions should be mostly wrapped incatchReflectException
orcatchNormalException
and turned intoEither[String, T]
sMinor Changes
I broke out
ExpandBraces.scala
andExpandBracesTests.scala
, so they're no longer mixed in withParseArgs.scala
. Mill's brace expansion happens textually in a totally separate phase before argument parsing runs, following how the Bash shell performs brace expansion, and so neither query parsing nor brace expansion need to know about each other.Segments
API was tweaked a bit to better suite the common usage patternsTesting
MainTests
was renamedResolveTests
, and now exercises theResolveMetadata
code path in addition toResolveTasks
ResolveTests
has a new sectionmoduleInitError
, which goes through a number of scenarios - three standalone modules, two dependent modules, partially and fully-broken cross module - and checks that module initialization failures in various places are properly caught and handled with truncated stack traces shown to usersAdded a simple integration test
integration/failure/module-init-error
to exercise the error handling during module initialization end-to-end and make sure that truncated stack traces are shown to usersAdded some more tests to
ResolveTests.scala
, covering the end-to-end handling of brace expansion, along with error reporting for double-nested modules (to reproduce a previous bug)Notes
The API to task resolution used by most code internal and external should be mostly the same, the only minor change is from
RunScript.resolveTasks(mill.main.ResolveFoo, ...)
toResolveFoo.resolve(...)
due to the removal of one layer of indirectionI might have missed some spots where we need to catch exceptions to truncate/convert-to-either; the nature of exceptions means there's no way to statically guarantee you caught them all. But the tests should cover most use cases, and if we later notice some code paths still throwing uncaught exceptions it'll be easy to wrap them then
The laziness of module initialization can probably be improved further, e.g. we don't need to initialize modules at all for
mill resolve
unless there are cross modules whose keys we need to compute. Leaving that for future work