Hotfix/remove null this work around (#831)

* Re-enable warnings around null this.

* Remove testing for nullptr in Substitution::Equals tests

* Fix ref counting problem in vulkan render.

* * Remove SLANG_ASSERT(this) in mthods
* Place asserts conservatively at method call sites where appropriate.

6 files changed, 53 insertions, 40 deletions

diff --git a/premake5.lua b/premake5.lua
index 7bb84291e..dc3faa392 100644
--- a/premake5.lua
+++ b/premake5.lua
@@ -115,13 +115,13 @@ workspace "slang"
         architecture "ARM"
 
     filter { "toolset:clang or gcc*" }
-        buildoptions { "-Wno-unused-parameter", "-Wno-type-limits", "-Wno-sign-compare", "-Wno-unused-variable", "-Wno-reorder", "-Wno-switch", "-Wno-return-type", "-Wno-unused-local-typedefs", "-Wno-parentheses",  "-std=c++11", "-fvisibility=hidden" , "-fno-delete-null-pointer-checks", "-Wno-ignored-optimization-argument", "-Wno-unknown-warning-option"} 
+        buildoptions { "-Wno-unused-parameter", "-Wno-type-limits", "-Wno-sign-compare", "-Wno-unused-variable", "-Wno-reorder", "-Wno-switch", "-Wno-return-type", "-Wno-unused-local-typedefs", "-Wno-parentheses",  "-std=c++11", "-fvisibility=hidden" , "-Wno-ignored-optimization-argument", "-Wno-unknown-warning-option"} 
     	
 	filter { "toolset:gcc*"}
-		buildoptions { "-Wno-nonnull-compare", "-Wno-unused-but-set-variable", "-Wno-implicit-fallthrough"  }
+		buildoptions { "-Wno-unused-but-set-variable", "-Wno-implicit-fallthrough"  }
 		
     filter { "toolset:clang" }
-         buildoptions { "-Wno-deprecated-register", "-Wno-tautological-compare", "-Wno-missing-braces", "-Wno-undefined-var-template", "-Wno-unused-function", "-Wno-undefined-bool-conversion"}
+         buildoptions { "-Wno-deprecated-register", "-Wno-tautological-compare", "-Wno-missing-braces", "-Wno-undefined-var-template", "-Wno-unused-function"}
 		
     -- When compiling the debug configuration, we want to turn
     -- optimization off, make sure debug symbols are output,
diff --git a/source/slang/parameter-binding.cpp b/source/slang/parameter-binding.cpp
index 583d4fe54..ba5a68341 100644
--- a/source/slang/parameter-binding.cpp
+++ b/source/slang/parameter-binding.cpp
@@ -2259,7 +2259,10 @@ static RefPtr<TypeLayout> computeEntryPointParameterTypeLayout(
     RefPtr<VarLayout>               paramVarLayout,
     EntryPointParameterState&       state)
 {
-    auto paramType = GetType(paramDeclRef)->Substitute(typeSubst).as<Type>();
+    auto paramDeclRefType = GetType(paramDeclRef);
+    SLANG_ASSERT(paramDeclRefType);
+
+    auto paramType = paramDeclRefType->Substitute(typeSubst).as<Type>();
 
     if( paramDeclRef.getDecl()->HasModifier<HLSLUniformModifier>() )
     {
@@ -2489,6 +2492,7 @@ static void collectEntryPointParameters(
     //
     for( auto taggedUnionType : entryPoint->taggedUnionTypes )
     {
+        SLANG_ASSERT(taggedUnionType);
         auto substType = taggedUnionType->Substitute(typeSubst).as<Type>();
         auto typeLayout = CreateTypeLayout(context->layoutContext, substType);
         entryPointLayout->taggedUnionTypeLayouts.Add(typeLayout);
@@ -2594,6 +2598,8 @@ static void collectEntryPointParameters(
     // types and apply this logic unconditionally.
     //
     auto resultType = GetResultType(entryPointFuncDeclRef)->Substitute(typeSubst).as<Type>();
+    SLANG_ASSERT(resultType);
+
     if( !resultType->Equals(resultType->getSession()->getVoidType()) )
     {
         state.loc = entryPointFuncDeclRef.getLoc();
@@ -3106,9 +3112,10 @@ RefPtr<ProgramLayout> specializeProgramLayout(
 
         // In the case where things are generic-dependent, we need to re-do
         // the type layout process on the type that results from doing
-        // substutition with the global generic arguments.
+        // substitution with the global generic arguments.
         //
         RefPtr<Type> oldType = oldVarLayout->getTypeLayout()->getType();
+        SLANG_ASSERT(oldType);
         RefPtr<Type> newType = oldType->Substitute(typeSubst).as<Type>();
 
         RefPtr<TypeLayout> newTypeLayout = getTypeLayoutForGlobalShaderParameter(
diff --git a/source/slang/syntax-base-defs.h b/source/slang/syntax-base-defs.h
index b1faf6776..b0da3f57e 100644
--- a/source/slang/syntax-base-defs.h
+++ b/source/slang/syntax-base-defs.h
@@ -32,7 +32,7 @@ ABSTRACT_SYNTAX_CLASS(Val, NodeBase)
     // substitutions to this one
     RefPtr<Val> Substitute(SubstitutionSet subst);
 
-    // Lower-level interface for substition. Like the basic
+    // Lower-level interface for substitution. Like the basic
     // `Substitute` above, but also takes a by-reference
     // integer parameter that should be incremented when
     // returning a modified value (this can help the caller
@@ -118,14 +118,13 @@ ABSTRACT_SYNTAX_CLASS(Substitutions, RefObject)
 
     // Check if these are equivalent substitutiosn to another set
     virtual bool Equals(Substitutions* subst) = 0;
-    virtual bool operator == (const Substitutions & subst) = 0;
     virtual int GetHashCode() const = 0;
     )
 END_SYNTAX_CLASS()
 
 SYNTAX_CLASS(GenericSubstitution, Substitutions)
     // The generic declaration that defines the
-    // parametesr we are binding to arguments
+    // parameters we are binding to arguments
     DECL_FIELD(GenericDecl*, genericDecl)
 
     // The actual values of the arguments
@@ -137,10 +136,7 @@ SYNTAX_CLASS(GenericSubstitution, Substitutions)
 
     // Check if these are equivalent substitutiosn to another set
     virtual bool Equals(Substitutions* subst) override;
-    virtual bool operator == (const Substitutions & subst) override
-    {
-        return Equals(const_cast<Substitutions*>(&subst));
-    }
+
     virtual int GetHashCode() const override
     {
         int rs = 0;
@@ -169,10 +165,7 @@ SYNTAX_CLASS(ThisTypeSubstitution, Substitutions)
 
     // Check if these are equivalent substitutiosn to another set
     virtual bool Equals(Substitutions* subst) override;
-    virtual bool operator == (const Substitutions & subst) override
-    {
-        return Equals(const_cast<Substitutions*>(&subst));
-    }
+
     virtual int GetHashCode() const override;
     )
 END_SYNTAX_CLASS()
@@ -201,10 +194,7 @@ RAW(
 
     // Check if these are equivalent substitutiosn to another set
     virtual bool Equals(Substitutions* subst) override;
-    virtual bool operator == (const Substitutions & subst) override
-    {
-        return Equals(const_cast<Substitutions*>(&subst));
-    }
+
     virtual int GetHashCode() const override
     {
         int rs = actualType->GetHashCode();
@@ -242,7 +232,7 @@ ABSTRACT_SYNTAX_CLASS(Modifier, SyntaxNode)
     )
 END_SYNTAX_CLASS()
 
-// A syntax node which can have modifiers appled
+// A syntax node which can have modifiers applied
 ABSTRACT_SYNTAX_CLASS(ModifiableSyntaxNode, SyntaxNode)
 
     SYNTAX_FIELD(Modifiers, modifiers)
diff --git a/source/slang/syntax.cpp b/source/slang/syntax.cpp
index 0af318c9e..709206278 100644
--- a/source/slang/syntax.cpp
+++ b/source/slang/syntax.cpp
@@ -330,7 +330,7 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
 
 
-    bool ArrayExpressionType::EqualsImpl(Type * type)
+    bool ArrayExpressionType::EqualsImpl(Type* type)
     {
         auto arrType = as<ArrayExpressionType>(type);
         if (!arrType)
@@ -444,8 +444,13 @@ void Type::accept(IValVisitor* visitor, void* extra)
             }
 
         case RequirementWitness::Flavor::val:
-            return RequirementWitness(
-                getVal()->Substitute(subst));
+            {
+                auto val = getVal();
+                SLANG_ASSERT(val);
+
+                return RequirementWitness(
+                    val->Substitute(subst));
+            }
         }
     }
 
@@ -1336,8 +1341,6 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
     RefPtr<Substitutions> GenericSubstitution::applySubstitutionsShallow(SubstitutionSet substSet, RefPtr<Substitutions> substOuter, int* ioDiff)
     {
-        SLANG_ASSERT(this);
-
         int diff = 0;
 
         if(substOuter != outer) diff++;
@@ -1361,8 +1364,11 @@ void Type::accept(IValVisitor* visitor, void* extra)
     bool GenericSubstitution::Equals(Substitutions* subst)
     {
         // both must be NULL, or non-NULL
-        if (!this || !subst)
-            return !this && !subst;
+        if (subst == nullptr)
+            return false;
+        if (this == subst)
+            return true;
+
         auto genericSubst = as<GenericSubstitution>(subst);
         if (!genericSubst)
             return false;
@@ -1388,8 +1394,6 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
     RefPtr<Substitutions> ThisTypeSubstitution::applySubstitutionsShallow(SubstitutionSet substSet, RefPtr<Substitutions> substOuter, int* ioDiff)
     {
-        SLANG_ASSERT(this);
-
         int diff = 0;
 
         if(substOuter != outer) diff++;
@@ -1409,9 +1413,10 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
     bool ThisTypeSubstitution::Equals(Substitutions* subst)
     {
-        SLANG_ASSERT(this);
         if (!subst)
             return false;
+        if (subst == this)
+            return true;
 
         if (auto thisTypeSubst = as<ThisTypeSubstitution>(subst))
         {
@@ -1462,6 +1467,9 @@ void Type::accept(IValVisitor* visitor, void* extra)
     {
         if (!subst)
             return false;
+        if (subst == this)
+            return true;
+
         if (auto genSubst = as<GlobalGenericParamSubstitution>(subst))
         {
             if (paramDecl != genSubst->paramDecl)
@@ -1485,13 +1493,16 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
     RefPtr<Type> DeclRefBase::Substitute(RefPtr<Type> type) const
     {
+        // Note that type can be nullptr, and so this function can return nullptr (although only correctly when no substitutions) 
+
         // No substitutions? Easy.
         if (!substitutions)
             return type;
 
+        SLANG_ASSERT(type);
+
         // Otherwise we need to recurse on the type structure
         // and apply substitutions where it makes sense
-
         return type->Substitute(substitutions).as<Type>();
     }
 
@@ -1555,6 +1566,7 @@ void Type::accept(IValVisitor* visitor, void* extra)
         RefPtr<Substitutions>   restSubst,
         int*                    ioDiff)
     {
+        SLANG_ASSERT(substToSpecialize);
         return substToSpecialize->applySubstitutionsShallow(substsToApply, restSubst, ioDiff);
     }
 
@@ -1949,7 +1961,6 @@ void Type::accept(IValVisitor* visitor, void* extra)
 
     RefPtr<Val> Val::Substitute(SubstitutionSet subst)
     {
-        SLANG_ASSERT(this);
         if (!subst) return this;
         int diff = 0;
         return SubstituteImpl(subst, &diff);
@@ -2437,11 +2448,16 @@ void Type::accept(IValVisitor* visitor, void* extra)
         return name->text;
     }
 
-    bool SubstitutionSet::Equals(SubstitutionSet substSet) const
+    bool SubstitutionSet::Equals(const SubstitutionSet& substSet) const
     {
-        if(!substitutions || !substSet.substitutions)
-            return substitutions == substSet.substitutions;
-
+        if (substitutions == substSet.substitutions)
+        {
+            return true;
+        }
+        if (substitutions == nullptr || substSet.substitutions == nullptr)
+        {
+            return false;
+        }
         return substitutions->Equals(substSet.substitutions);
     }
 
diff --git a/source/slang/syntax.h b/source/slang/syntax.h
index 344e94ff9..6a404214e 100644
--- a/source/slang/syntax.h
+++ b/source/slang/syntax.h
@@ -417,7 +417,7 @@ namespace Slang
             : substitutions(subst)
         {
         }
-        bool Equals(SubstitutionSet substSet) const;
+        bool Equals(const SubstitutionSet& substSet) const;
         int GetHashCode() const;
     };
 
diff --git a/tools/gfx/render-vk.cpp b/tools/gfx/render-vk.cpp
index 32bc91de4..e291c7e9c 100644
--- a/tools/gfx/render-vk.cpp
+++ b/tools/gfx/render-vk.cpp
@@ -2505,7 +2505,7 @@ void VKRenderer::setDescriptorSet(PipelineType pipelineType, PipelineLayout* lay
 
 Result VKRenderer::createProgram(const ShaderProgram::Desc& desc, ShaderProgram** outProgram)
 {
-    ShaderProgramImpl* impl = new ShaderProgramImpl(desc.pipelineType);
+    RefPtr<ShaderProgramImpl> impl = new ShaderProgramImpl(desc.pipelineType);
     if( desc.pipelineType == PipelineType::Compute)
     {
         auto computeKernel = desc.findKernel(StageType::Compute);
@@ -2519,7 +2519,7 @@ Result VKRenderer::createProgram(const ShaderProgram::Desc& desc, ShaderProgram*
         impl->m_vertex = compileEntryPoint(*vertexKernel, VK_SHADER_STAGE_VERTEX_BIT, impl->m_buffers[0]);
         impl->m_fragment = compileEntryPoint(*fragmentKernel, VK_SHADER_STAGE_FRAGMENT_BIT, impl->m_buffers[1]);
     }
-    *outProgram = impl;
+    *outProgram = impl.detach();
     return SLANG_OK;
 }