Make the AST nodes a package

[thewilsonator]

No description provided.

[rikkimax]

Failed to compile the `unit` test executable! (exit code 1)

> /home/runner/work/dmd/dmd/generated/linux/release/64/dmd -version=NoBackend -version=GC -version=NoMain -version=MARS -version=DMDLIB -g -unittest -J/home/runner/work/dmd/dmd/generated/linux/release/64 -Jsrc/dmd/res -Isrc -I/home/runner/work/dmd/dmd/compiler/test/unit -i -main -of/home/runner/work/dmd/dmd/compiler/test/test_results/runner -m64 test/unit/frontend.d test/unit/interfaces/check_implementations_20861.d test/unit/lexer/location_offset.d test/unit/lexer/lexer_dmdlib.d test/unit/lexer/diagnostic_reporter.d test/unit/triple.d test/unit/self_test.d test/unit/support.d test/unit/compilable/searching.d test/unit/compilable/crlf.d test/unit/semantic/control_flow.d test/unit/semantic/imports.d test/unit/semantic/covariance.d test/unit/deinitialization.d test/unit/parser/test_astbase.d test/unit/parser/diagnostic_reporter.d test/unit/parser/visibilitydeclaration_location.d test/unit/parser/linkage_location.d test/unit/parser/aliasdeclaration_location.d test/unit/parser/conditionalcompilation_location.d test/unit/parser/dvcondition_location.d test/unit/objc/protocols/optional_methods.d test/unit/objc/protocols/diagnostic_messages.d /home/runner/work/dmd/dmd/compiler/test/test_results/runner.d
test/unit/compilable/searching.d(12): Error: unable to read module `dclass`
import dmd.dclass : ClassDeclaration;
       ^
test/unit/compilable/searching.d(12):        Expected 'dmd/dclass.d' or 'dmd/dclass/package.d' in one of the following import paths:
import dmd.dclass : ClassDeclaration;
       ^
import path[0] = src
import path[1] = /home/runner/work/dmd/dmd/compiler/test/unit
import path[2] = /home/runner/work/dmd/dmd/generated/linux/release/64/../../../../druntime/import
import path[3] = /home/runner/work/dmd/dmd/generated/linux/release/64/../../../../../phobos

[rikkimax]

Possibly? https://github.com/thewilsonator/dmd/blob/5dfe0000f196bf25b64f6f7087de0339604d3e56/compiler/src/build.d#L965

For DAutoTest failure.

[rikkimax]

test\unit\parser\dvcondition_location.d(5): Expected 'dmd\cond.d' or 'dmd\cond\package.d' in one of the following import paths:

[rikkimax]

test\unit\support.d(57): Expected 'dmd\dmodule.d' or 'dmd\dmodule\package.d' in one of the following import paths:

[rikkimax]

test/unit/deinitialization.d(78): Expected 'dmd/dmodule.d' or 'dmd/dmodule/package.d' in one of the following import paths:

[thewilsonator]

oh, frontend.h, bleh whole file needs to be replaced.

[ibuclaw]

And what justification is there for this constantly breaking projects that have been using dmd as a library for over 20 years?

[thewilsonator]

This is the culmination of four(?) years of work, three (4?) GSoC/SAoC projects.

DMD explicitly does not guarantee API stability across version and breaks compatibility all the time. This change doesn't even change function signatures, and is functionally identical to the packaging of dmd.visitor #16875, the downstream corrective action is to rename imports.

[dlang-bot]

Thanks for your pull request and interest in making D better, @thewilsonator! 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

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

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 "master + dmd#23098"

[dkorpel]

Sorry, I gotta agree with Iain here.

• This is very disruptive
• Sunk cost is not a justification
• The goal of the AST refactor was to get rid of astbase.d, not to move code
• We had agreed with Walter that we would stop packaging frontend modules after the ones he reluctantly let in

Can we please let this rest now?

[thewilsonator]

This is very disruptive

Only marginally so. It is more disruptive than say #16875, #21572 , #21394,
#16907, #16909, but not by a huge amount. Yes this will require a bunch of PRs to need rebases, and downstream users of DMD as a library will need to enter a few ast. into their imports, and are one time costs and par for the course w.r.t DMD API (in)stability. The benefits by contrast are ongoing.

Sunk cost is not a justification

Indeed, it should pull its weight, and IMO it does. The refactorings leading up to this were all an improvement, and this is the last step, and a significant portion of the navigability wins. I am of the opinion that this is a great improvement to the structural navigability of the codebase, especially for newcomers.

The goal of the AST refactor was to get rid of astbase.d, not to move code

That was one goal. It has always been my goal to do this and I think I've made that pretty clear. Also this does not merely "move code", it makes the codebase structure apparent in the directory structure.

We had agreed with Walter that we would stop packaging frontend modules after the ones he reluctantly let in

#21394 (comment)

As I recall (though admittedly not in great detail) was that he didn't want it done prematurely, but was fine for the other packages (mangle, glue, iasm, etc.) because their import graphs were as isolated and sparse as they were going to be. Since then we have had a SAoC student make a very valiant effort in separating out the semantic stuff and I have put in some more work cleaning things up.

Can we please let this rest now?

If there is some timeframe after which this is more acceptable, e.g. after a release codifying editions, then I am all ears and I am happy to twist some arms over beer at dconf for this, but I am not going to let this go and I would prefer to get this done sooner rather than later.

[dkorpel]

Well as it stands the PR still has 146 files changed, 30 KLOC moved, but an empty description. Only the term "structural navigability" is dropped somewhere in this conversation. To start, this PR should have a description saying exactly what it does. Which files were moved into the ast package, and why?

Also, compiler/src/dmd/README.md should be updated and document exactly what the ast package means, and what property modules must have to be in there. E.g. "the ast package is compilable on its own", or "modules in the ast package don't import any XXXsem.d modules", or "the parser is compilable using only the ast package". Btw currently none of these properties hold, so I don't know what the current philosophy of the ast package is. Why is astnode.d in there but not rootobject.d / identifier.d?

Then, the value of those properties can be weighed against the disruption to see if this PR indeed pulls it's weight. Because a tree structure doesn't automatically mean better navigation than a flat structure, especially if the branches are chosen somewhat arbitrarily.