Skip to content

Fix issue #1283#1855

Merged
tritao merged 4 commits intomono:mainfrom
HE2-SDK:bugfix/issue-1283
Jul 26, 2024
Merged

Fix issue #1283#1855
tritao merged 4 commits intomono:mainfrom
HE2-SDK:bugfix/issue-1283

Conversation

@angryzor
Copy link
Copy Markdown
Contributor

@angryzor angryzor commented Jul 26, 2024

See #1283 .

GetterSetterToPropertyPass currently fails on the following code:

namespace foo {
class Dorld {
    virtual int UnkFunc1() = 0;
};
}

class Bar : public foo::Dorld {
public:
    virtual int UnkFunc1() override;
};

Resulting output

The namespace is important. If Bar is declared inside the same namespace the bug is not triggered. However, my original code where I encountered this bug did have both classes in the same namespace, though they were in different header files.

The bug occurs when it tries to determine whether the property overrides a base property here.

When Bar is outside the namespace, Bar gets processed before Dorld, so Dorld doesn't have the property created yet. The check fails and the property is not created on Bar. The property is then later created on Dorld when the pass finally visits Dorld.

When instead Bar is inside the namespace, Dorld is visited first and the code continues as expected.

Fixed by setting the VisitFlags.ClassBases flag on the pass.

@angryzor
Copy link
Copy Markdown
Contributor Author

angryzor commented Jul 26, 2024

@dotnet-policy-service agree

@angryzor angryzor changed the title Fixes issue #1283 Fix issue #1283 Jul 26, 2024
@tritao
Copy link
Copy Markdown
Collaborator

tritao commented Jul 26, 2024

Thanks so much for looking into this, any chance you could add a test to the test suite?

@angryzor
Copy link
Copy Markdown
Contributor Author

angryzor commented Jul 26, 2024

Thanks so much for looking into this, any chance you could add a test to the test suite?

Sure, here you go! (I hope I added it in the correct place 😅)

@tritao
Copy link
Copy Markdown
Collaborator

tritao commented Jul 26, 2024

We usually have these in https://github.com/mono/CppSharp/blob/main/tests/dotnet/CSharp/CSharp.Tests.cs / https://github.com/mono/CppSharp/blob/main/tests/dotnet/CSharp/CSharp.h which actually does end-to-end testing, but I guess there is nothing inherently wrong with your approach, its just gives a bit less guarantees.

@angryzor
Copy link
Copy Markdown
Contributor Author

angryzor commented Jul 26, 2024 •
edited
Loading

Agh, I had it there first, then saw there was a suite for the passes. Alright, I'll move it over. The strange thing is though, this bug prevents compilation on my machine, so I expected TestUncompilableCode to fail when I added the fixture, but that didn't happen. because I didn't undo my fix first. Never mind!

@angryzor
Copy link
Copy Markdown
Contributor Author

angryzor commented Jul 26, 2024 •
edited
Loading

So with the test in the new position, the build just fails and the tests can't even be run when the issue is present:

1>------ Build started: Project: CSharp.CSharp, Configuration: Release x64 ------
1>Skipping analyzers to speed up the build. You can execute 'Build' or 'Rebuild' command to run analyzers.
1><...>\CSharp\CSharp.cs(37969,33,37969,72): error CS0534: 'TestOverrideOfPropertyInNamespacedClass' does not implement inherited abstract member 'HasOverridenPropertyInNamespacedClass.Property.get'
1>Done building project "CSharp.CSharp.csproj" -- FAILED.

Is that acceptable?

@tritao
Copy link
Copy Markdown
Collaborator

tritao commented Jul 26, 2024

I think so, there seems to be some delay in running the macOS CI, but looks good otherwise.

@tritao tritao merged commit b658ff3 into mono:main Jul 26, 2024
@tritao
Copy link
Copy Markdown
Collaborator

tritao commented Jul 26, 2024 •
edited
Loading

The mac CI is still not running, but since its already passing on other platforms, I will just merge it. Again, thanks for the fix.

@angryzor
Copy link
Copy Markdown
Contributor Author

angryzor commented Jul 26, 2024

The mac CI is still not running, but since its already passing on other platforms, I will just merge it. Again, thanks for the fix.

No problem!

@angryzor angryzor deleted the bugfix/issue-1283 branch July 26, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@angryzor @tritao