Change default visibility of interface members and update docs. (#3381)

* Update behavior around interfaces and docs.

* Update toc

---------

Co-authored-by: Yong He <yhe@nvidia.com>

10 files changed, 100 insertions, 61 deletions

diff --git a/docs/user-guide/03-convenience-features.md b/docs/user-guide/03-convenience-features.md
index 3155cd1b5..74676699f 100644
--- a/docs/user-guide/03-convenience-features.md
+++ b/docs/user-guide/03-convenience-features.md
@@ -395,50 +395,3 @@ by using the `[ForceInline]` decoration:
 [ForceInline]
 int f(int x) { return x + 1; }
 ```
-
-Modules
--------
-
-While you can still organize code using preprocessor `#include`s, Slang also supports a _module_ system.
-
-### Importing a Module
-
-At the global scope of a Slang file, you can use the `import` keyword to import another module by name:
-
-```hlsl
-// MyShader.slang
-
-import YourLibrary;
-```
-
-This `import` declaration will cause the compiler to look for a module named `YourLibrary` and make its declarations visible in the current scope.
-Currently, the compiler will load the module by looking for source file with a matching name (`YourLibrary.slang`) in any of its configured search paths.
-
-Multiple `import`s of the same module from different input files will only cause the module to be loaded once (there is no need for "include guards" or `#pragma once`).
-Note that preprocessor definitions in the current file will not affect the compilation of `import`ed code.
-
-> #### Note ####
-> Future versions of the Slang system will support loading of modules from pre-compiled binaries instead of source code.
-> The same `import` keyword will continue to work in that case.
-
-### Defining a Module
-
-If you decide to organize your code into modules, then the simplest way is to have each module consist of a single `.slang` file.
-Any declarations in your module -- types, functions, etc. -- will be visible to clients that `import` it.
-
-> #### Note ####
-> Any preprocessor definitions inside your module will not be visible to clients;
-> as a result you may find it best to switch to `static const` for defining constants.
-
-> #### Note ####
-> A future version of the Slang compiler may support using access-control keywords (such a `public`) to control which declarations in a module are visible to clients.
-
-Your module may depend on other modules using `import`.
-By default, the symbols that are imported into your module are *not* made visible to clients who `import` your module.
-You can override this default by using an _exported_ `import`:
-
-```hlsl
-__export import SomeOtherModule;
-```
-
-This line imports `SomeOtherModule` into the current module, and also re-exports all of the imported symbols from the current module, so that they appear as part of its public interface.
diff --git a/docs/user-guide/04-modules-and-access-control.md b/docs/user-guide/04-modules-and-access-control.md
index 5beb2b197..1dd766aff 100644
--- a/docs/user-guide/04-modules-and-access-control.md
+++ b/docs/user-guide/04-modules-and-access-control.md
@@ -6,10 +6,10 @@ layout: user-guide
 Modules and Access Control
 ===========================
 
-While the preprocessor `#include`s is still supported, Slang provides a _module_ system for software engineering benefits such as clean expression of sub-component boundaries and dependencies, hiding implementation details, and providing a path towards true separate compilation.
+While the preprocessor `#include`s is still supported, Slang provides a _module_ system for software engineering benefits such as clean expression of sub component boundaries and dependencies, hiding implementation details, and providing a path towards true separate compilation.
 
 
-### Defining a Module
+## Defining a Module
 
 A module in Slang comprises one or more files. A module must have one and only one primary file that is used as the source-of-truth to uniquely identify the module. The primary file must start with `module` declaration. For example, the following code defines a module named `scene`:
 
@@ -83,7 +83,7 @@ __include "dir/file-name";
 > #### Note ####
 > When using the identifier token syntax, Slang will translate any underscores(`_`) to hyphenators("-") to obtain the file name.
 
-### Importing a Module
+## Importing a Module
 
 At the global scope of a Slang file, you can use the `import` keyword to import another module by name:
 
@@ -114,7 +114,7 @@ Note that preprocessor definitions in the current file will not affect the compi
 > Future versions of the Slang system will support loading of modules from pre-compiled binaries instead of source code.
 > The same `import` keyword will continue to work in that case.
 
-### Access Control
+## Access Control
 
 Slang supports access control modifiers: `public`, `internal` and `private`. The module boundary plays an important role in access control.
 
@@ -149,6 +149,11 @@ void outerFunc(MyType t)
 // a.slang
 module a;
 __include b;
+public struct PS
+{
+    int internalMember;
+    public int publicMember;
+}
 internal void f() { f_b(); } // OK, f_b defined in the same module.
 
 // b.slang
@@ -163,7 +168,31 @@ void main()
 {
     f(); // Error, f is not visible here.
     publicFunc(); // OK.
+    PS p; // OK.
+    p.internalMember = 1; // Error, internalMember is not visible.
+    p.publicMember = 1; // OK.
 }
 ```
 
-`internal` is the default visibility if no other access modifiers are specified.
+`internal` is the default visibility if no other access modifiers are specified, an exception is for `interface` members, where the default visibility is the visibility of the interface.
+
+### Additional Validation Rules
+
+The Slang compiler enforces the following rules regarding access control:
+- A more visible entity should not expose less visible entities through its signature. For example, a `public` function cannot have a return type that is `internal`.
+- A member of a `struct`, `interface` and other aggregate types cannot have a higher visibility than its parent.
+- If a `struct` type has visibility `Vs`, and one of its member has visibility `Vm`, and the member is used to satisfy an interface requirement that has visibility `Vr`, then `Vm` must not be lower (less visible) than `min(Vs, Vr)`.
+- Type definitions themselves cannot be `private`, for example, `private struct S {}` is not valid code.
+- `interface` requirements cannot be `private`.
+
+
+## Legacy Modules
+
+Slang used to not have support for access control, and all symbols were treated as having `public` visibility. To provide compatibility with existing code, the Slang compiler will detect if the module is written in the legacy language, and treat all symbols as `public` if so.
+
+A module is determined to be written in legacy language if all the following conditions are met:
+- The module is lacking `module` declaration at the begining.
+- There is no use of `__include`.
+- There is no use of any visibility modifiers -- `public`, `private` or `internal`.
+
+The user is advised that this legacy mode is for compatibility only. This mode may be deprecated in the future, and it is strongly recommended that new code should not rely on this compiler behavior.
\ No newline at end of file
diff --git a/docs/user-guide/toc.html b/docs/user-guide/toc.html
index 615b96b15..5f8a9d46a 100644
--- a/docs/user-guide/toc.html
+++ b/docs/user-guide/toc.html
@@ -42,10 +42,15 @@
 <li data-link="03-convenience-features#extensions"><span>Extensions</span></li>
 <li data-link="03-convenience-features#multi-level-break"><span>Multi-level break</span></li>
 <li data-link="03-convenience-features#force-inlining"><span>Force inlining</span></li>
-<li data-link="03-convenience-features#modules"><span>Modules</span></li>
 </ul>
 </li>
 <li data-link="04-modules-and-access-control"><span>Modules and Access Control</span>
+<ul class="toc_list">
+<li data-link="04-modules-and-access-control#defining-a-module"><span>Defining a Module</span></li>
+<li data-link="04-modules-and-access-control#importing-a-module"><span>Importing a Module</span></li>
+<li data-link="04-modules-and-access-control#access-control"><span>Access Control</span></li>
+<li data-link="04-modules-and-access-control#legacy-modules"><span>Legacy Modules</span></li>
+</ul>
 </li>
 <li data-link="05-interfaces-generics"><span>Interfaces and Generics</span>
 <ul class="toc_list">
diff --git a/source/slang/slang-ast-support-types.h b/source/slang/slang-ast-support-types.h
index 1e71d0c4a..f771e24b6 100644
--- a/source/slang/slang-ast-support-types.h
+++ b/source/slang/slang-ast-support-types.h
@@ -66,6 +66,17 @@ namespace Slang
     void printDiagnosticArg(StringBuilder& sb, Val* val);
     void printDiagnosticArg(StringBuilder& sb, DeclRefBase* declRefBase);
 
+    struct QualifiedDeclPath
+    {
+        DeclRefBase* declRef;
+        QualifiedDeclPath() = default;
+        QualifiedDeclPath(DeclRefBase* declRef)
+            : declRef(declRef)
+        {}
+    };
+    // Prints the fully qualified decl name.
+    void printDiagnosticArg(StringBuilder& sb, QualifiedDeclPath path);
+
 
     class SyntaxNode;
     SourceLoc getDiagnosticPos(SyntaxNode const* syntax);
@@ -1605,6 +1616,7 @@ namespace Slang
         Public,
         Default = Internal,
     };
+
 } // namespace Slang
 
 #endif
diff --git a/source/slang/slang-check-decl.cpp b/source/slang/slang-check-decl.cpp
index 671bb26dd..39f0b9096 100644
--- a/source/slang/slang-check-decl.cpp
+++ b/source/slang/slang-check-decl.cpp
@@ -4059,7 +4059,19 @@ namespace Slang
                 continue;
 
             if (doesMemberSatisfyRequirement(member.declRef, requiredMemberDeclRef, witnessTable))
+            {
+                // The member satisfies the requirement in every other way except that
+                // it may have a lower visibility than min(parentVisibility, requirementVisibilty),
+                // in that case we will treat it as an error.
+                auto minRequiredVisibility = Math::Min(getDeclVisibility(requiredMemberDeclRef.getDecl()), getTypeVisibility(subType));
+                if (getDeclVisibility(member.declRef.getDecl()) < minRequiredVisibility)
+                {
+                    getSink()->diagnose(member.declRef, Diagnostics::satisfyingDeclCannotHaveLowerVisibility, member.declRef);
+                    getSink()->diagnose(requiredMemberDeclRef, Diagnostics::seeDeclarationOf, QualifiedDeclPath(requiredMemberDeclRef));
+                    return false;
+                }
                 return true;
+            }
         }
 
         // If we reach this point then there were no members suitable
diff --git a/source/slang/slang-check-expr.cpp b/source/slang/slang-check-expr.cpp
index dd868f70c..9d1898cf5 100644
--- a/source/slang/slang-check-expr.cpp
+++ b/source/slang/slang-check-expr.cpp
@@ -841,6 +841,12 @@ namespace Slang
             else if (as<PrivateModifier>(modifier))
                 return DeclVisibility::Private;
         }
+        
+        // Interface members will always have the same visibility as the interface itself.
+        if (auto interfaceDecl = findParentInterfaceDecl(decl))
+        {
+            return getDeclVisibility(interfaceDecl);
+        }
 
         if (auto parentModule = getModuleDecl(decl))
             return parentModule->isInLegacyLanguage ? DeclVisibility::Public : DeclVisibility::Internal;
@@ -919,6 +925,13 @@ namespace Slang
         {
             getSink()->diagnose(loc, Diagnostics::declIsNotVisible, lookupResult.item.declRef);
             outDiagnosed = true;
+
+            if (getShared()->isInLanguageServer())
+            {
+                // When in language server mode, return the unfiltered result so we can still
+                // provide language service around it.
+                return lookupResult;
+            }
         }
         return result;
     }
diff --git a/source/slang/slang-diagnostic-defs.h b/source/slang/slang-diagnostic-defs.h
index c95d5ea98..ac3220f4b 100644
--- a/source/slang/slang-diagnostic-defs.h
+++ b/source/slang/slang-diagnostic-defs.h
@@ -360,7 +360,7 @@ DIAGNOSTIC(30505, Error, implementingMustReferencePrimaryModuleFile, "the source
 // Visibilty
 DIAGNOSTIC(30600, Error, declIsNotVisible, "'$0' is not accessible from the current context.")
 DIAGNOSTIC(30601, Error, declCannotHaveHigherVisibility, "'$0' cannot have a higher visibility than '$1'.")
-DIAGNOSTIC(30602, Error, declCannotHaveLowerVisibility, "'$0' cannot have a lower visibility than '$1'.")
+DIAGNOSTIC(30602, Error, satisfyingDeclCannotHaveLowerVisibility, "'$0' is less visible than the interface requirement it satisfies.")
 DIAGNOSTIC(30603, Error, invalidUseOfPrivateVisibility, "'$0' cannot have private visibility.")
 DIAGNOSTIC(30604, Error, useOfLessVisibleType, "'$0' references less visible type '$1'.")
 
diff --git a/source/slang/slang-syntax.cpp b/source/slang/slang-syntax.cpp
index 678d9ec26..6350a7e4d 100644
--- a/source/slang/slang-syntax.cpp
+++ b/source/slang/slang-syntax.cpp
@@ -2,7 +2,7 @@
 
 #include "slang-compiler.h"
 #include "slang-visitor.h"
-
+#include "slang-ast-print.h"
 #include <typeinfo>
 #include <assert.h>
 
@@ -55,6 +55,13 @@ void printDiagnosticArg(StringBuilder& sb, QualType const& type)
         sb << "<null>";
 }
 
+void printDiagnosticArg(StringBuilder& sb, QualifiedDeclPath path)
+{
+    ASTPrinter printer(getCurrentASTBuilder());
+    printer.addDeclPath(path.declRef);
+    sb << printer.getString();
+}
+
 SourceLoc getDiagnosticPos(SyntaxNode const* syntax)
 {
     if (!syntax)
diff --git a/source/slang/slang.cpp b/source/slang/slang.cpp
index 0f53ffcd5..2852dd43e 100644
--- a/source/slang/slang.cpp
+++ b/source/slang/slang.cpp
@@ -1117,12 +1117,6 @@ SLANG_NO_THROW slang::IModule* SLANG_MCALL Linkage::loadModuleFromSource(
             if (SLANG_SUCCEEDED(Path::getCanonical(pathStr, cannonicalPath)))
             {
                 pathInfo = PathInfo::makeNormal(pathStr, cannonicalPath);
-                ComPtr<ISlangBlob> uniqueIdentity;
-                getFileSystemExt()->getFileUniqueIdentity(cannonicalPath.getBuffer(), uniqueIdentity.writeRef());
-                if (uniqueIdentity && uniqueIdentity->getBufferSize() != 0)
-                {
-                    pathInfo.uniqueIdentity = (char*)uniqueIdentity->getBufferPointer();
-                }
             }
         }
         auto module = loadModule(
diff --git a/tests/diagnostics/internal-visibility/interface-default-visibility.slang b/tests/diagnostics/internal-visibility/interface-default-visibility.slang
new file mode 100644
index 000000000..79df14143
--- /dev/null
+++ b/tests/diagnostics/internal-visibility/interface-default-visibility.slang
@@ -0,0 +1,14 @@
+//DIAGNOSTIC_TEST:SIMPLE(filecheck=CHECK):
+
+module m;
+
+public interface IFoo
+{
+    void foo(); // Should have public visibility by default.
+}
+
+public struct F : IFoo
+{
+    // CHECK:{{.*}}(13): error 30602:
+    void foo() {};
+}