o3de / o3de-azslc

Amazon Shader Language (AZSL) Compiler
Other
23 stars 14 forks source link

Class inheritance #58

Closed siliconvoodoo closed 2 years ago

siliconvoodoo commented 2 years ago

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: https://github.com/microsoft/DirectXShaderCompiler/issues/4625 https://github.com/microsoft/DirectXShaderCompiler/issues/4629 (this is is relevant and to follow up)

jeremyong-az commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

Fixed both issues in #59 closing this branch.