Refactor duplicated code from ASTBase node AliasThis into a mixin template by RazvanN7 · Pull Request #12796 · dlang/dmd

RazvanN7 · 2021-07-01T15:54:09Z

This is mostly a proposal of getting rid of the duplication in ASTBase. If I get a green light on this, I plan to apply it for all the nodes in ASTBase.

cc @WalterBright

dlang-bot · 2021-07-01T15:56:00Z

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

UplinkCoder · 2021-07-01T15:59:42Z

This makes it hard to see what's going on.

RazvanN7 · 2021-07-01T16:00:22Z

@UplinkCoder What are you referring to exactly?

If you are referring to the fields and methods of the AST nodes, I agree that it is inconvenient that you need to check astmembers to see what's going on, but I would argue that it's a better alternative than having duplicated code. Also, our testing pipeline should still catch any situations where a field/method is defined in the AST node definition of ASTCodegen, when in fact it should be defined in the mixin template.

…plate

UplinkCoder · 2021-07-01T16:03:02Z

The constructor is suddenly not inspectable anymore.
Yes the code is boilerplate and it can be copy pasted.
But hidining it in a mixin template doesn't make it better.
also that hides the method from __traits(allMembers).

RazvanN7 · 2021-07-01T16:06:06Z

ASTBase has been a constant pain and is already out of sync with the nodes in ASTCodegen. Having the shared code in a single place is worth the extra effort to inspect a different file.

also that hides the method from __traits(allMembers).

Code that does this should be updated to recursively check for mixin templates anyway.

UplinkCoder · 2021-07-01T16:08:42Z

astbase should just be removed alltogether.
it serves no purpose.

ibuclaw · 2021-07-01T16:17:48Z

src/dmd/aliasthis.d

-        super(loc, null);    // it's anonymous (no identifier)
-        this.ident = ident;
-    }
+    mixin parseTimePropertiesAliasThis;


This doesn't really address the duplication though, does it? There are still two copies of AliasThis present, and every time a new AST node is introduced it must still be defined twice.

The only way we could eliminate the duplication would be to have the ASTCodegen classes inherit from the ASTBase classes, however, that would require multiple inheritance since a node must inherit its parse time counterpart but also its logical ancestor (for example AddExp would need to inherit ParseTimeAddExp and BinaryExp). This could technically be done by using inheritance and alias this, but I would rather not go there since this currently isn't defined behavior (and ideally alias this would be deprecated for classes).

Instead, this PR limits the scope of the duplication. Indeed, new AST node declarations still need to be duplicated, but I would argue that this doesn't happen very often.

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

Why not? We've already removed the semantic, semantic2, semantic3, resolve, toIR, toCtype, toElem, toDt, and probably many more this way.

There are pointless member functions:

dmd/src/dmd/statement.d

Lines 216 to 249 in 0ff19fb

final bool usesEH()

{

extern (C++) final class UsesEH : StoppableVisitor

{

alias visit = typeof(super).visit;

public:

override void visit(Statement s)

{

}

override void visit(TryCatchStatement s)

{

stop = true;

}

override void visit(TryFinallyStatement s)

{

stop = true;

}

override void visit(ScopeGuardStatement s)

{

stop = true;

}

override void visit(SynchronizedStatement s)

{

stop = true;

}

}

scope UsesEH ueh = new UsesEH();

return walkPostorder(this, ueh);

}

There doesn't need to be a big clear-out done, just anything that isn't to do with examining itself. e.g. from Statement:

dmd/src/dmd/statement.h

Lines 119 to 129 in 0ff19fb

virtual Statement *getRelatedLabeled() { return this; }

virtual bool hasBreak() const;

virtual bool hasContinue() const;

bool usesEH();

bool comeFrom();

bool hasCode();

virtual Statement *scopeCode(Scope *sc, Statement **sentry, Statement **sexit, Statement **sfinally);

virtual Statements *flatten(Scope *sc);

virtual Statement *last();

virtual ReturnStatement *endsWithReturnStatement() { return NULL; }

Remove the above highlighted functions, and astbase can just publicly import dmd.statement...

Unfortunately, it's not that easy. Looking at statement.d it imports a lot of modules that don't have anything to do with parsing. Before being able to import statement.d we need to get rid of all of those dependencies. That is no easy task and it requires a lot of refactorings.

I'd be surprised if it stays that way after dealing with the above functions.

The problem is that statement.d has many dependencies. Even if we move out everything that relies on semantic information from statement.d, we will still have some Expression fields. expression.d has other dependencies that rely on semantic information, so now we need to the same for it too etc. My point is that, before we can simply do import statement; in ASTBase, we need to do this cleanup for all the AST nodes, otherwise the import dependencies are going to drag semantic information into ASTBase.

kinke · 2021-07-01T16:39:08Z

astbase should just be removed alltogether.
it serves no purpose.

I've never understood what it's for. Is something actually using this file at all? We exclude it for LDC, so this refactoring would make the LDC frontend actually look much uglier than it has to.

RazvanN7 · 2021-07-01T16:41:31Z

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

benjones · 2021-07-01T16:44:18Z

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

Looks like a few projects use it? https://github.com/search?q=import+dmd.astbase&type=code

kinke · 2021-07-01T16:51:35Z

Thx Razvan and Ben; after quickly glancing at the D-filtered results, it looks like 2 projects remain - https://github.com/dragospetrescu/SHDP-Parser/blob/5f01aaff66fd0d632fce421a7378a6fc138283e6/source/worker.d and https://github.com/DmitryOlshansky/dlang-graal/blob/27c672d74124530e9abef6be5d204601c3ecd0cc/dtool.d, besides some DMD dub package tests like https://github.com/dlang/dmd/blob/master/test/dub_package/avg.d.

Edit: And these 2 projects seem very incomplete/abandoned.

Geod24 · 2021-07-02T01:37:55Z

@kinke It was initially implemented to be able to separate the parser from the semantic analysis, so that users of dmd as a lib have the capability of building and extending solely the parser. I don't know if any projects are actually using it.

It's the first instance I can recall of designing a library for the sake of it. There's already libdparse, and associated libraries, if one wants to just parse D code.

And for the record, using mixins to remove duplication is like painting on a rusty car.

RazvanN7 · 2021-07-02T08:20:55Z

I'm trying to find a fix here to remove the duplication in ASTBase, but it looks like every direction I take encounters resistance. I suggest we start a discussion on what the best path is and stick with it, otherwise, this duplication will continue to exist.

We have 3 options:

Pull out everything that has something to do with semantic from AST nodes. That way, we end up with classes that simply contain some fields and methods for self examination. This is what I think to be the best option, however, when we tried it [1], it was rejected.
The mixin hack that this PR currently employs. I agree that this is just a workaround, but at least it removes the duplication of methods and fields, which IMHO is a step forward because we get rid of the inconsistencies that have already piled up. However, this option is also rejected.
Completely remove ASTBase. I agree that things are not ideal right now, but what ASTBase enables is building the compiler without any semantic dependencies. This is a step forward for dmd compiler as a library; a small step, but nevertheless a step forward. I don't see this as a viable alternative and if a PR were made to do so, that would be rejected also.

If there are any other alternatives, I am open to discussions, but one thing is certain: we need to get rid of this duplication. I would very much appreciate it if we have a debate on what is the best course of action, rather then just turn down any attempt at fixing the situation.

[1] #11788

RazvanN7 · 2021-07-02T08:23:14Z

It's the first instance I can recall of designing a library for the sake of it. There's already libdparse, and associated libraries, if one wants to just parse D code.

Yes, libdparse which is continually left behind by new frontend additions. Just take a look at what's happening right now: lidbparse has a memory corruption bug and we cannot update dscanner to unclogg the phobos PR queue. This would not have happened if dmd as a lib was used.

kinke · 2021-07-02T11:40:32Z

I know of 3 major projects using DMD as a library - GDC, LDC and Visual D, and none of them use ASTBase. So I'm clearly for option 3, removing it. Edit: Short-term, that is.

WalterBright · 2021-07-08T00:10:00Z

I'd like to explore something along the lines of an abstract interface, as that's the more traditional way of doing it rather than boilerplate insertion. The interface can use static polymorphism (aka template) or dynamic polmorphism (aka abstract class).

WalterBright · 2021-07-08T00:10:32Z

The abstract interface should not have any data members.

Refactor duplicated code from ASTBase node AliasThis into a mixin tem…

d1ecb95

…plate

RazvanN7 force-pushed the Dupl_dsymbol branch from d2357d1 to d1ecb95 Compare July 1, 2021 16:01

ibuclaw reviewed Jul 1, 2021

View reviewed changes

benjones mentioned this pull request Jul 2, 2021

move scopeCode to statementSem #12803

Merged

RazvanN7 closed this Jul 13, 2021

	final bool usesEH()
	{
	extern (C++) final class UsesEH : StoppableVisitor
	{
	alias visit = typeof(super).visit;
	public:
	override void visit(Statement s)
	{
	}

	override void visit(TryCatchStatement s)
	{
	stop = true;
	}

	override void visit(TryFinallyStatement s)
	{
	stop = true;
	}

	override void visit(ScopeGuardStatement s)
	{
	stop = true;
	}

	override void visit(SynchronizedStatement s)
	{
	stop = true;
	}
	}

	scope UsesEH ueh = new UsesEH();
	return walkPostorder(this, ueh);
	}

	virtual Statement *getRelatedLabeled() { return this; }
	virtual bool hasBreak() const;
	virtual bool hasContinue() const;
	bool usesEH();
	bool comeFrom();
	bool hasCode();
	virtual Statement scopeCode(Scope sc, Statement sentry, Statement sexit, Statement **sfinally);
	virtual Statements flatten(Scope sc);
	virtual Statement *last();

	virtual ReturnStatement *endsWithReturnStatement() { return NULL; }

Uh oh!

Conversation

RazvanN7 commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dlang-bot commented Jul 1, 2021

Bugzilla references

Testing this PR locally

Uh oh!

UplinkCoder commented Jul 1, 2021

Uh oh!

RazvanN7 commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UplinkCoder commented Jul 1, 2021

Uh oh!

RazvanN7 commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

UplinkCoder commented Jul 1, 2021

Uh oh!

ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

Uh oh!

RazvanN7 Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kinke commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Jul 1, 2021

Uh oh!

benjones commented Jul 1, 2021

Uh oh!

kinke commented Jul 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Geod24 commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RazvanN7 commented Jul 2, 2021

Uh oh!

RazvanN7 commented Jul 2, 2021

Uh oh!

kinke commented Jul 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

WalterBright commented Jul 8, 2021

Uh oh!

WalterBright commented Jul 8, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Comments

RazvanN7 commented Jul 1, 2021 •

edited

Loading

RazvanN7 commented Jul 1, 2021 •

edited

Loading

RazvanN7 commented Jul 1, 2021 •

edited

Loading

RazvanN7 Jul 2, 2021 •

edited

Loading

kinke commented Jul 1, 2021 •

edited

Loading

kinke commented Jul 1, 2021 •

edited

Loading

Geod24 commented Jul 2, 2021 •

edited

Loading

kinke commented Jul 2, 2021 •

edited

Loading