Move `newScope` out of AST nodes to `dsymbolsem.d` #16880

dchidindu5 · 2024-09-25T07:21:04Z

SAOC 24

I've got several errors but I'm interested in how to solve this one

src/dmd/dsymbolsem.d(7557): Error: no property `newScope` for `super` of type `dmd.visitor.Visitor`
src/dmd/dsymbolsem.d(7557):        the following error occured while looking for a UFCS match

Any suggestion as too how to fix them?
What am I doing wrong?


src/dmd/dsymbolsem.d(7557): Error: no property `newScope` for `super` of type `dmd.visitor.Visitor`
src/dmd/dsymbolsem.d(7557):        the following error occured while looking for a UFCS match
src/dmd/dsymbolsem.d(7557): Error: function `newScope` is not callable using argument types `(Visitor, Scope*)`
src/dmd/dsymbolsem.d(7557):        cannot pass argument `super` of type `dmd.visitor.Visitor` to parameter `Dsymbol d`
src/dmd/dsymbolsem.d(7478):        `dmd.dsymbolsem.newScope(Dsymbol d, Scope* sc)` declared here
src/dmd/dsymbolsem.d(7578): Error: undefined identifier `stc`, did you mean variable `sc`?

dlang-bot · 2024-09-25T07:21:07Z

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

compiler/src/dmd/dsymbolsem.d

dchidindu5 · 2024-09-25T21:47:18Z

Initially createNewScope was a returned statement but after the functions was turned into a visitor nothing was returned.
defined it locally but it didn't work.
Can I get a little explanation why it's like that? @thewilsonator

src/dmd/dsymbolsem.d(7520): Error: undefined identifier createNewScope``

compiler/src/dmd/dsymbolsem.d

dchidindu5 · 2024-09-26T14:24:53Z

Thanks @thewilsonator , the issues were solved but some thing happened along the line
Let me take this as an example
Original function before turning into a visitor

override Scope* newScope(Scope* sc)
    {
        if (ident == Id.Pinline)
        {
            // We keep track of this pragma inside scopes,
            // then it's evaluated on demand in function semantic
            return createNewScope(sc, sc.stc, sc.linkage, sc.cppmangle, sc.visibility, sc.explicitVisibility, sc.aligndecl, this);
        }
        return sc;
    }

After it was turned into a visitor

override void visit(PragmaDeclaration prd)
    {
        if (prd.ident == Id.Pinline)
        {
            // We keep track of this pragma inside scopes,
            // then it's evaluated on demand in function semantic
            createNewScope(sc, sc.stc, sc.linkage, sc.cppmangle, sc.visibility, sc.explicitVisibility, sc.aligndecl, this);
        }
        return;
    }

then this was done

sc = prd.createNewScope(sc, sc.stc, sc.linkage, sc.cppmangle, sc.visibility, sc.explicitVisibility, sc.aligndecl, this);

an error occured

src/dmd/dsymbolsem.d(7536): Error: function `dmd.attrib.AttribDeclaration.createNewScope(Scope* sc, ulong stc, LINK linkage, CPPMANGLE cppmangle, Visibility visibility, int explicitVisibility, AlignDeclaration aligndecl, PragmaDeclaration inlining)` is not callable using argument types `(Scope*, ulong, LINK, CPPMANGLE, Visibility, int, AlignDeclaration, newScopeVisitor)`
src/dmd/dsymbolsem.d(7537):        cannot pass argument `this` of type `dmd.dsymbolsem.newScopeVisitor` to parameter `PragmaDeclaration inlining`

This is the only error for now I simply do not understand fully.

thewilsonator · 2024-09-27T00:56:33Z

whoops, this should be replaced by the object being visited, prd

dchidindu5 · 2024-09-27T23:27:24Z

the compiler has been built successfully but I need to run the test suite @thewilsonator

thewilsonator · 2024-09-28T00:45:42Z

OK, so push your changes that fix compilation, and the CI will run (I think I or someone else will need to approve it since you are still a first time committer here).

dchidindu5 · 2024-09-29T01:36:15Z

@thewilsonator I ran the test suite for dmd and this occured

posix.mak:1: ===== DEPRECATION NOTICE ===== 
posix.mak:2: ===== DEPRECATION: posix.mak is deprecated. Please use generic Makefile instead.
posix.mak:3: ============================== 
make -f Makefile unittest
make[1]: Entering directory '/home/dencomac/DL/dmd'
Using D host compiler: dmd
make[1]: *** No rule to make target 'unittest'.  Stop.
make[1]: Leaving directory '/home/dencomac/DL/dmd'
make: *** [posix.mak:11: unittest] Error 2

I noticed the posix.mak is deprecated. so I tried this

make -f Makefile unittest

and this error occurred

Using D host compiler: dmd
make: *** No rule to make target 'unittest'.  Stop.

Does that mean that the make file is missing in the current directory.?

thewilsonator · 2024-09-29T01:40:05Z

No idea, try ./run.d? Either way, push your changes and the CI will run them.

dchidindu5 · 2024-09-29T01:57:30Z

okayy

compiler/src/dmd/dsymbolsem.d

dchidindu5 · 2024-09-30T19:22:44Z

I've implemented the changes and I've closed up the indentations

compiler/src/dmd/dsymbolsem.d

dchidindu5 · 2024-10-03T01:16:40Z

I actually implement Razvan's suggestion here #16880 (comment)

compiler/src/dmd/dsymbolsem.d

dchidindu5 · 2024-10-03T13:35:15Z

make -f posix.mak
posix.mak:1: ===== DEPRECATION NOTICE =====
posix.mak:2: ===== DEPRECATION: posix.mak is deprecated. Please use generic Makefile instead.
posix.mak:3: ==============================
make -f Makefile all
make[1]: Entering directory '/home/dencomac/DL/dmd'
Using D host compiler: dmd
generated/build dmd
Success
make -C druntime
make[2]: Entering directory '/home/dencomac/DL/dmd/druntime'
../generated/linux/release/64/dmd -c -conf= -Isrc -Iimport -w -de -preview=dip1000 -preview=fieldwise -m64 -fPIC -preview=dtorfields -O -release -inline -version=Shared -fPIC -version=Shared -fPIC -I. -P=-I. src/core/stdc/errno.c -of../generated/linux/release/64/errno_c.o
make[2]: *** [Makefile:394: ../generated/linux/release/64/errno_c.o] Segmentation fault
make[2]: Leaving directory '/home/dencomac/DL/dmd/druntime'
make[1]: *** [Makefile:89: druntime] Error 2
make[1]: Leaving directory '/home/dencomac/DL/dmd'
make: *** [posix.mak:8: all] Error 2

…actice1

thewilsonator

Looking good!

compiler/src/dmd/dsymbolsem.d

thewilsonator · 2024-10-04T13:41:23Z

compiler/src/dmd/dsymbol.h

@@ -448,4 +448,5 @@ namespace dmd
    Dsymbol *search(Dsymbol *d, const Loc &loc, Identifier *ident, SearchOptFlags flags = (SearchOptFlags)SearchOpt::localsOnly);
    void setScope(Dsymbol *d, Scope *sc);
    void importAll(Dsymbol *d, Scope *sc);
+    Scope* newScope(Dsymbol *d, Scope *sc);


...in which case remove this from this header

if for some reason it needs to be, you will also need to update frontend.h with the same changes (look for a failure on the CircleCI tester)

Razvan said I only should remove newScope from attrib.h @thewilsonator

Yes, but that doesn't mean you need add it here. You must do one of two things:

leave newScope extern(C++), leave this dsymbol.haddition here and add it tofrontend.h` (see the Circle CI failure)

make newScope extern(D) and remove this addition (and don't update frontend.h, because there is nothing that needs updating).

newScope is unused by LDC and from what I can tell is unused by GDC, so there is no need for it to be extern(C++).

it says "newly generated header file (/home/circleci/dmd/generated/linux/release/64/frontend.h) doesn't match with the reference header file (/home/circleci/dmd/compiler/src/dmd/frontend.h) and The file src/dmd/frontend.h seems to be out of sync. This is likely because changes were made which affect the C++ interface used by GDC and LDC.

I think I got it now

Yes, this is because newScope is extern(C++). See also the diff that is says you should apply:

diff --git a/home/circleci/dmd/compiler/src/dmd/frontend.h b/home/circleci/dmd/generated/linux/release/64/frontend.h index 23d404ba6b..9e08f3cd0b 100644 --- a/home/circleci/dmd/compiler/src/dmd/frontend.h +++ b/home/circleci/dmd/generated/linux/release/64/frontend.h @@ -6214,7 +6214,6 @@ class AttribDeclaration : public Dsymbol public: Array<Dsymbol* >* decl; virtual Array<Dsymbol* >* include(Scope* sc); - virtual Scope* newScope(Scope* sc); void addComment(const char* comment) override; const char* kind() const override; bool oneMember(Dsymbol*& ps, Identifier* ident) override; @@ -6231,7 +6230,6 @@ class StorageClassDeclaration : public AttribDeclaration public: StorageClass stc; StorageClassDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; bool oneMember(Dsymbol*& ps, Identifier* ident) final override; StorageClassDeclaration* isStorageClassDeclaration() override; void accept(Visitor* v) override; @@ -6243,7 +6241,6 @@ public: Expression* msg; const char* msgstr; DeprecatedDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; void accept(Visitor* v) override; }; @@ -6253,7 +6250,6 @@ public: LINK linkage; static LinkDeclaration* create(const Loc& loc, LINK p, Array<Dsymbol* >* decl); LinkDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* toChars() const override; void accept(Visitor* v) override; }; @@ -6263,7 +6259,6 @@ class CPPMangleDeclaration final : public AttribDeclaration public: CPPMANGLE cppmangle; CPPMangleDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* toChars() const override; void accept(Visitor* v) override; }; @@ -6273,7 +6268,6 @@ class CPPNamespaceDeclaration final : public AttribDeclaration public: Expression* exp; CPPNamespaceDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* toChars() const override; void accept(Visitor* v) override; CPPNamespaceDeclaration* isCPPNamespaceDeclaration() override; @@ -6285,7 +6279,6 @@ public: Visibility visibility; _d_dynamicArray< Identifier* > pkg_identifiers; VisibilityDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* kind() const override; const char* toPrettyChars(bool __param_0_) override; VisibilityDeclaration* isVisibilityDeclaration() override; @@ -6298,7 +6291,6 @@ public: Array<Expression* >* exps; structalign_t salign; AlignDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; void accept(Visitor* v) override; }; @@ -6321,7 +6313,6 @@ class PragmaDeclaration final : public AttribDeclaration public: Array<Expression* >* args; PragmaDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* kind() const override; void accept(Visitor* v) override; }; @@ -6374,7 +6365,6 @@ class ForwardingAttribDeclaration final : public AttribDeclaration public: ForwardingScopeDsymbol* sym; ForwardingAttribDeclaration(Array<Dsymbol* >* decl); - Scope* newScope(Scope* sc) override; ForwardingAttribDeclaration* isForwardingAttribDeclaration() override; void accept(Visitor* v) override; }; @@ -6396,7 +6386,6 @@ class UserAttributeDeclaration final : public AttribDeclaration public: Array<Expression* >* atts; UserAttributeDeclaration* syntaxCopy(Dsymbol* s) override; - Scope* newScope(Scope* sc) override; const char* kind() const override; void accept(Visitor* v) override; }; @@ -7407,6 +7396,8 @@ public: void visit(StaticForeachDeclaration* _) override; }; +extern Scope* newScope(Dsymbol* d, Scope* sc); + class NrvoWalker final : public StatementRewriteWalker { public:

Note that you should leave out that last addition if you make newScope extern(D)

thewilsonator · 2024-10-05T00:21:23Z

Note that you still need to remove the newScope from frontend.h as noted in the diff #16880 (comment)

I think that is the last thing that you need to do for this.

dchidindu5 · 2024-10-05T01:07:39Z

Note that you still need to remove the newScope from frontend.h as noted in the diff #16880 (comment)

I think that is the last thing that you need to do for this.

I deleted newScope from frontend.h and attrib.h added newScope in dsymbol.h (Scope* newScope(Dsymbol *d, Scope *sc);
changed newScope to extern(D) Scope* newScope

Right??

changes done locally only

thewilsonator · 2024-10-05T01:12:54Z

newScope (the one that was in attrib, there are another set of functions with the same name elsewhere) should not be in any header file, because you have made it extern(D), that function doesn't exist as for as C++ in concerned.

dchidindu5 · 2024-10-05T01:38:28Z

newScope (the one that was in attrib, there are another set of functions with the same name elsewhere) should not be in any header file, because you have made it extern(D), that function doesn't exist as for as C++ in concerned.

can I go on with the commit??

thewilsonator · 2024-10-05T01:41:40Z

can I go on with the commit??

I'm not sure what you mean there, but this has to pass CI for me to merge it. And Circle tests that frontend.h is up to date.

dchidindu5 · 2024-10-05T01:49:39Z

can I go on with the commit??

I'm not sure what you mean there, but this has to pass CI for me to merge it. And Circle tests that frontend.h is up to date.

I mean I've done the changes and I want to update the commit

thewilsonator · 2024-10-05T01:51:05Z

You don't need to ask permission to do that.

compiler/src/dmd/frontend.h

dchidindu5 added 9 commits September 12, 2024 04:39

extract newScope to dsymbolsem

c01f76b

Extract newScope functions to dsymbolsem

b2300c0

Batch 3 errors encountered

83ba750

Merge branch 'dlang:master' into practice1

8ed06f1

Extract newScope and turn to visitor

36489c9

Merge branch 'dlang:master' into practice1

b9eab55

Extract newScope and turn into a visitor

21de527

Merge branch 'dlang:master' into practice1

311414f

C

cd797ca

thewilsonator reviewed Sep 25, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

thewilsonator reviewed Sep 25, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

thewilsonator changed the title ~~Draft PR - Missing: no property newScope for super of type dmd.visitor.Visitor~~ Move newScope out of AST nodes to dsymbolsem.d Sep 27, 2024

Move newScope out of AST nodes to dsymbolsem.d

b1c7e9c

dchidindu5 marked this pull request as ready for review September 29, 2024 22:41

thewilsonator reviewed Sep 30, 2024

View reviewed changes

Move newScope out of AST nodes to dsymbolsem.d

fd7f928

thewilsonator reviewed Sep 30, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

Move newScope out of AST nodes to dsymbolsem.d

e9c9c7e

thewilsonator reviewed Oct 1, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

Merge branch 'dlang:master' into practice1

d65d3f7

Move newScope out of AST nodes to dsymbolsem.d

f6d9314

RazvanN7 reviewed Oct 3, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

RazvanN7 reviewed Oct 3, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

RazvanN7 and others added 5 commits October 3, 2024 16:51

Fix segfault when compiling druntime

78c9e42

Merge branch 'master' into practice1

063ccba

Remove newScope from attrib.h

02757c9

Merge branch 'dlang:master' into practice1

f5e9fc9

Merge branch 'practice1' of https://github.com/dchidindu5/dmd into pr…

15bab03

…actice1

dchidindu5 requested a review from ibuclaw as a code owner October 4, 2024 12:57

thewilsonator reviewed Oct 4, 2024

View reviewed changes

compiler/src/dmd/dsymbolsem.d Outdated Show resolved Hide resolved

Remove whitespace

f788b09

thewilsonator reviewed Oct 4, 2024

View reviewed changes

make newScope extern(D)

e0f76cd

dchidindu5 closed this Oct 5, 2024

dchidindu5 reopened this Oct 5, 2024

Remove newScope from frontend.h

9ec7a80

thewilsonator reviewed Oct 5, 2024

View reviewed changes

compiler/src/dmd/frontend.h Outdated Show resolved Hide resolved

compiler/src/dmd/frontend.h Outdated Show resolved Hide resolved

compiler/src/dmd/frontend.h Outdated Show resolved Hide resolved

Fix newScope in frontend.h

4b0e00b

thewilsonator approved these changes Oct 5, 2024

View reviewed changes

thewilsonator merged commit f8846a7 into dlang:master Oct 5, 2024
41 checks passed

thewilsonator pushed a commit to thewilsonator/dmd that referenced this pull request Oct 7, 2024

Move newScope out of AST nodes to dsymbolsem.d (dlang#16880)

d4a0921

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move `newScope` out of AST nodes to `dsymbolsem.d` #16880

Move `newScope` out of AST nodes to `dsymbolsem.d` #16880

dchidindu5 commented Sep 25, 2024

dlang-bot commented Sep 25, 2024

dchidindu5 commented Sep 25, 2024

dchidindu5 commented Sep 26, 2024

thewilsonator commented Sep 27, 2024

dchidindu5 commented Sep 27, 2024

thewilsonator commented Sep 28, 2024

dchidindu5 commented Sep 29, 2024

thewilsonator commented Sep 29, 2024

dchidindu5 commented Sep 29, 2024

dchidindu5 commented Sep 30, 2024

dchidindu5 commented Oct 3, 2024

dchidindu5 commented Oct 3, 2024

thewilsonator left a comment

thewilsonator Oct 4, 2024

thewilsonator Oct 4, 2024

dchidindu5 Oct 4, 2024

thewilsonator Oct 4, 2024

dchidindu5 Oct 4, 2024

dchidindu5 Oct 4, 2024

thewilsonator Oct 4, 2024 •

edited

Loading

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024 •

edited

Loading

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024

thewilsonator commented Oct 5, 2024

Move newScope out of AST nodes to dsymbolsem.d #16880

Move newScope out of AST nodes to dsymbolsem.d #16880

Conversation

dchidindu5 commented Sep 25, 2024

SAOC 24

dlang-bot commented Sep 25, 2024

Bugzilla references

Testing this PR locally

dchidindu5 commented Sep 25, 2024

dchidindu5 commented Sep 26, 2024

thewilsonator commented Sep 27, 2024

dchidindu5 commented Sep 27, 2024

thewilsonator commented Sep 28, 2024

dchidindu5 commented Sep 29, 2024

thewilsonator commented Sep 29, 2024

dchidindu5 commented Sep 29, 2024

dchidindu5 commented Sep 30, 2024

dchidindu5 commented Oct 3, 2024

dchidindu5 commented Oct 3, 2024

thewilsonator left a comment

Choose a reason for hiding this comment

thewilsonator Oct 4, 2024

Choose a reason for hiding this comment

thewilsonator Oct 4, 2024

Choose a reason for hiding this comment

dchidindu5 Oct 4, 2024

Choose a reason for hiding this comment

thewilsonator Oct 4, 2024

Choose a reason for hiding this comment

dchidindu5 Oct 4, 2024

Choose a reason for hiding this comment

dchidindu5 Oct 4, 2024

Choose a reason for hiding this comment

thewilsonator Oct 4, 2024 • edited Loading

Choose a reason for hiding this comment

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024 • edited Loading

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024

thewilsonator commented Oct 5, 2024

dchidindu5 commented Oct 5, 2024

thewilsonator commented Oct 5, 2024

Move `newScope` out of AST nodes to `dsymbolsem.d` #16880

Move `newScope` out of AST nodes to `dsymbolsem.d` #16880

thewilsonator Oct 4, 2024 •

edited

Loading

dchidindu5 commented Oct 5, 2024 •

edited

Loading