Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Class inheritance #58

Closed
wants to merge 10 commits into from

Conversation

siliconvoodoo
Copy link
Contributor

@siliconvoodoo siliconvoodoo commented Aug 31, 2022

The commit messages, the code, and the tests are conveying some messages relevant to understand the gritty details of that PR.
The general description is that this PR activates support for class inheritance, as it was semantically disabled before, and not supported by internals.

Most potentially impactful change is the adaptation of the LookupSymbol function that is now resolving inherited symbols (creates name resolution links between children and parent that are transparent to the caller).

Tested: multiple inheritance, diamond inheritance, mixed with interface, deeper than 1 level, symbol hiding, qualified id expression access (a::b), member access expression (a.b), and hiding-unveiling MAE (a.B::b).
Verified that the seenat feature still tracks occurences of fields referenced anywhere in the program.

Investigated C++ and DXC behavior on the way for various scenarios.
Here is a few of the captures of that analysis:
class-inheritance-Investigation-deep-interface
class-inheritance-investigation-function-differ-return-types
class-inheritance-investigation-inaccessible-base
class-inheritance-investigation-member-access-ok
class-inheritance-investigation-same-name-in-nesting-cxxforbidden

2 DXC bugs found along the way, reported:
microsoft/DirectXShaderCompiler#4625
microsoft/DirectXShaderCompiler#4629 (this is is relevant and to follow up)

  • The test suite passes
  • Pasting a fresh release exe in the Builder folders, and touching CommonPBR.azlsi seemed to trigger a fair amount of shaders that passed the build.

+ verify early that pyyaml isn't missing. this causes 20 fails.
+ trim whitespace

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…s recursively).

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
+ add re-emission of interfaces
+ add emission of inheritance lists
+ change base holding from set to vector (to preserve order)
+ fixup base lookup scope (it was target class's scope, must be enclosing)
+ add a typeof case to exploit inheritance lookup

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…ed members, and also fully qualified idExpression that were bypassing lookup.
wip: advanced / verification of seenats not ready

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
… nameless.

Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
…declared prior to a nested type, ended up extracted ABOVE the definition of their own parent type.
Signed-off-by: Vivien Oddou <vivien.oddou@siliconstudio.co.jp>
Copy link
Contributor

@jeremyong-az jeremyong-az left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great thanks! FYI, there are some commits that aren't signed which is a requirement for O3DF (open 3d foundation) code. There are instructions here on how to fix it (you'll need to rebase your changes as signed commits with the signature in the commit body).

@jeremyong-az
Copy link
Contributor

Do we need tests regarding multiple inheritance btw? Looking into HLSL a bit more, it would seem that multiple inheritance isn't supported (see the upcoming proposal: https://clang.llvm.org/docs/HLSL/HLSLSupport.html for example)

@siliconvoodoo
Copy link
Contributor Author

siliconvoodoo commented Sep 5, 2022

Do we need tests regarding multiple inheritance btw? Looking into HLSL a bit more, it would seem that multiple inheritance isn't supported (see the upcoming proposal: https://clang.llvm.org/docs/HLSL/HLSLSupport.html for example)

So to be precise, in that case Azslc doesn't support supplementary features to DXC because there is no significant transpilation happening around the inheritance specification.
(The re-emission attempts a reconstruction of the AST, and the AST isn't mutated, only analyzed and partially checked.)
The "Multi inheritance" test refers to interface which DXC supports. What DXC doesn't support is multi class inheritance.
Azslc has its own error that reproduces that semantic constraint, to limit the risk of bugs with symbol lookup:

class A{};
class B{};
class C : A, B {};

output:

(3,0) : Semantic error #17: class C has multiple concrete bases. Only 1 concrete base allowed

This is indeed missing a test though. I'll add one tomorrow!

@siliconvoodoo
Copy link
Contributor Author

Fixed both issues in #59
closing this branch.

@siliconvoodoo siliconvoodoo deleted the class_inheritance branch September 6, 2022 08:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants