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 resolution of children of override lazy val modules #3270

Merged
merged 3 commits into from
Jul 18, 2024

Conversation

lihaoyi
Copy link
Member

@lihaoyi lihaoyi commented Jul 18, 2024

This was preventing resolution of modules in the form override lazy val foo: FooModule = new FooModule{...}, which are necessary when you want to override one module with another. You couldn't resolve them via resolve or run them from the command line, but you could depend on them from other modules and have then get picked up by planning logic

These turn up in Mill's own build, and I notice I couldn't resolve e.g. main.codesig.test.cases[callgraph-basic-1-static-method].compile before this PR. After this PR, I can.

Filtering out abstract methods in Reflect seems unnecessary, since compilation checks that every method is implemented already.

Added a unit test to cover this edge case.

Pull request: #3270

@lihaoyi lihaoyi requested review from lefou and lolgab July 18, 2024 08:06
Copy link
Member

@lefou lefou left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lihaoyi lihaoyi merged commit 6ff0e6c into com-lihaoyi:main Jul 18, 2024
39 checks passed
@lefou lefou added this to the 0.11.10 milestone Jul 18, 2024
lefou added a commit that referenced this pull request Jul 31, 2024
Newer versions of Mill better find these nested modules
(#3270), so that our CI check
for formatted Scala files will fail once we bump to Mill `0.11.10` or
newer.

Pull request: #3323
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.

2 participants