Remove ASTNull

[yebblies]

This removes a module added in #6625 - see my comment there for more details on why.

After reviewing this in more detail the conclusion is that there is no use case for the ASTNull AST family that is not better covered by ASTBase and therefore should be removed.

While this does cover the API used by the parser to construct an AST it does not actually result in building an AST, and at the same time is ineffective for use cases that don't require an AST as it will allocate huge amounts of memory and does not provide a hookable API. The testing of the parser API will be adequately covered by ASTBase.

[mathias-lang-sociomantic]

Would be appreciated if you could add the comment to the commit as well, so people tracing history don't have to do "blame" => commit => P.R. => Issue => Comment

[jacob-carlborg]

Yes, the commit message should contain a reason.

[yebblies]

How would deleted lines show up in blame?

[yebblies]

That said I'm willing to add any text to the commit message in exchange for a speedy merge.

[jacob-carlborg]

I would be more happy with a reason in the commit message, but I don't have any authority to merge.

[yebblies]

I'm hesitant to put "because it's useless and should never have been merged" into the permanent history.

[jacob-carlborg]

I'm hesitant to put "because it's useless and should never have been merged" into the permanent history.

Sure, but it all depends on how you word it. I think what you wrote in the PR that introduced the ASTNull was a pretty good explanation, or at least parts of it. Here's a suggestion for a commit message that is based on what you wrote in #6625 (comment).

Remove the ASTNull AST family

After reviewing this in more detail the conclusion is that there is
no use case for the ASTNull AST family and therefore should be
removed.

A minimal AST family is useful to have, but the minimal AST is
ASTBase, not this. For it to be a minimal AST it must by definition
have some kind of use. Making sure that no non-semantic fields are
accessed is covered by ASTBase. The minimal AST set that does
involve a class hierarchy must be capable (at the very least) of
representing the tree structure of parsed nodes.

[jacob-carlborg]

Also, if you write the explanation in the commit message (and have only one commit), GitHub will automatically use that commit message as the title and description of the PR, avoiding the need to refer to the other PR.

[yebblies]

Honestly I'm expecting someone with merge rights to come along and request a slightly different comment from what you asked for, at which point I'll update it and they'll merge it.

[andralex]

It is my judgment (as the initiator and co-designer of this project) that the ASTNull family has valid uses. I have a number of ideas on how truncated ASTs can be useful, and others may have similar or distinct ideas. Things can be improved further by not allocating truncated nodes at all, but for such ideas to take shape the initial notion must be there. Please let's keep this in. Thanks.

[ibuclaw]

I agree with this, however I was not included in the discussion of this AST and how this carp got merged I have no idea.

I'm hesitant to put "because it's useless and should never have been merged" into the permanent history.

I'll merge if you put this, or any other suggested commit message.

[ibuclaw]

@yebblies - tentative ping.

[andralex]

OK, to be considered when there's a good use case for it. (One issue is it's not visitable which reduces usefulness.)

[ibuclaw]

@andralex do we have a forum to discuss this? Maybe the internals ML. As I've said before, I don't think the parser is something that needs speeding up or has memory problems. For plugging in your own passes/analysers, there's already the Visitor interface, which is used by gdc.

[jacob-carlborg]

@andralex actually, I did use this ASTNull family in my PR that adds a Dub file #6771. It's used in a simple test to verify that everything needed to create the parser is available. Not sure if that use case is enough.

[ibuclaw]

@jacob-carlborg - You could do the exact same with the normal parser?

[jacob-carlborg]

You could do the exact same with the normal parser?

@ibuclaw you mean the ASTCodegen family? That would bring in the whole compiler as depending modules.

[andralex]

@jacob-carlborg could you use ASTBase instead?

[andralex]

@ibuclaw internals is fine as is skype. There are benefits to the decomposition that go well beyond speed or memory consumption (which I see more of a perk than a fundamental pursuit). By modularizing the compiler:

• we can distribute small. simple, and uncluttered libraries for those interested in a subset of the compiler
• we are "forced" to define and maintain clean interfaces between modules, which end up being our interfaces for the users of the compiler as a library
• we can improve and test in separation distinct functional units
• we make it easier for newcomers to understand the architecture of the compiler
• ideally we allow others to extend the compiler with new processing steps

I.e.. the usual advantages of modularization. Perhaps it's clear to you who are familiar with the codebase, but even the parser was a rat's nest of dependencies - for example, one surprising thing is that parsing depends on the name mangler. There is an explanation for it, but it's one of many surprising consequences of the compiler being monolithic.

[wilzbach]

@ibuclaw internals is fine as is skype.

FWIW there's also dlang.slack.com (however one needs an invite with the free version)

[jacob-carlborg]

could you use ASTBase instead?

@andralex yes, I didn't know that existed. I found it now in a PR.