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 22730 - Promote imported files (-i) to root module before parsing the declarations. #13654

Merged

Conversation

MoonlightSentinel
Copy link
Contributor

#13224 changed the parser s.t. unittests from non-root modules are skipped, i.e. not even parsed. The new behaviour didn't work as expected when combined with -i because it promoted imported files to root modules after the parser processed the entire file.

The first commit splits the module parsing into dedicated methods for the module declaration and content. The second commit then moves the existing checks s.t. they are applied immediatly after the module declaration and before the first unittest.

@dlang-bot
Copy link
Contributor

dlang-bot commented Feb 15, 2022

Thanks for your pull request and interest in making D better, @MoonlightSentinel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

  • My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
  • My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
  • I have provided a detailed rationale explaining my changes
  • New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.


If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close Bugzilla Severity Description
22730 regression master: "dmd -i" doesn't include unit tests from imported modules

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "stable + dmd#13654"

@MoonlightSentinel MoonlightSentinel changed the base branch from master to stable February 15, 2022 02:26
@MoonlightSentinel MoonlightSentinel added the Regression PRs that fix regressions label Feb 15, 2022
@MoonlightSentinel MoonlightSentinel marked this pull request as draft February 15, 2022 02:31
@MoonlightSentinel MoonlightSentinel marked this pull request as ready for review February 15, 2022 02:44
Copy link
Member

@Geod24 Geod24 left a comment

Choose a reason for hiding this comment

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

I'm not sure I follow the logic. Why is the static assert in lib.ignores.unittests not triggering ?

src/dmd/parse.d Outdated Show resolved Hide resolved
S.t. the `module x.y.z` can be parsed independent of the module content.
This is required to eagerly determine whether an imported files should
be promoted to a root module when compiling with `-i`. (it's currently
done after the entire module was parsed).
... the declarations.

DMD PR 13224 changed the parser s.t. unittests from non-root modules
are skipped, i.e. not even parsed. The new behaviour didn't work as
expected when combined with `-i` because it promoted imported files to
root modules *after* the parser processed the entire file.

This commit moves the existing checks s.t. they are applied immediatly
after the module declaration was read by the parser.
@MoonlightSentinel
Copy link
Contributor Author

MoonlightSentinel commented Feb 15, 2022

Why is the static assert in lib.ignores.unittests not triggering ?

imports.skipped_unittest_lib doesn't match any -i=<pattern> and hence remains a non-root module, causing the parser to skip any unittest found within the file.

@MoonlightSentinel
Copy link
Contributor Author

I've changed the test to a compilable example s.t. errors won't be hidden if the order of semantic evaluation is changed. (i.e. the "good" static assert(false) in the root module triggers before that non-root one)

@thewilsonator thewilsonator merged commit 1c31d85 into dlang:stable Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-merge Bug Fix Regression PRs that fix regressions
Projects
None yet
4 participants