Fix the way extension declarations are cached for lookup (#1450)

During semantic checking, the compiler used to link together `ExtensionDecl`s into a singly-linked list dangling off of the `AggTypeDecl` that they applied to. This approach made lookup relatively easy, because given a `DeclRef` to an `AggTypeDecl` one could easily find and walk the list of candidate extensions.

Unfortunately, the simple approach has two major strikes against it:

* First, as we recently ran into, it creates a lifetime/ownership problem, in cases where the `ExtensionDecl` is outlived by the `AggTypeDecl` it applies to. This creates the one and only place in the compiler today where an "old" AST node might point to a "new" AST node, and it resulted in use-after-free problems in client code.

* Second, the scoping of `extension`s ends up being completely wrong. All of the `extension` methods on a type end up being visible in all cases, instead of just in the context of modules where the `extension` itself is visible. The comparable feature in C# (static extension methods) is careful to not make scoping mistakes like this. The Swift langauge has loose scoping for `extension` more akin to what we have in Slang today, but the maintainers seem to consider it a misfeature.

This change attempts to clean up both issues by changing the way that extension declarations are stored. There are two main pieces:

1. The primary "source of truth" for extension lookup has been moved to the `ModuleDecl`, where a module is responsible for storing a cache of the extensions declared within that module (keyed by the declaration of the type being extended). This cache is updated at the same point where the old code would mutate the AST node being depended on.

2. A secondary aggregated cache is added to the `SharedSemanticsContext` used during semantic checking. This cache includes entries from across multiple modules, and is intended to be invalidated and rebuilt on demand if new modules are added during checking.

Access to the candidate extensions has now been put behind subroutines that require a semantics-checking context to be passed in (there was always one available in contexts that care about extensions).

In addition, the operation for looking up members including those from extensions was refactored heavily to involve internal rather than external iteration and, more importantly, was changed so that it actually tests whether the `ExtensionDecl`s it loops over apply to the type in question, rather than blindly letting extensions members be looked up in ways that don't make sense.

There are three test cases added here to confirm aspects of the fix:

* First, I added a test that reproduces the crash that was being seen, so that we have a regression test for the fix.

* Second, I added a basic semantic-checking test to confirm that an `extension` from an `import`ed module is still visible/usable, to confirm that I didn't break existing valid uses of extensions.

* Third, I added a diagnostic test that ensures that we correctly ignore extensions that should not be visible in a given context as a result of `import` declarations.

Co-authored-by: jsmall-nvidia <jsmall@nvidia.com>

8 files changed, 130 insertions, 0 deletions

diff --git a/tests/bugs/extension-lifetime.slang b/tests/bugs/extension-lifetime.slang
new file mode 100644
index 000000000..38cdf580b
--- /dev/null
+++ b/tests/bugs/extension-lifetime.slang
@@ -0,0 +1,41 @@
+// extension-lifetime.slang
+
+// This test is a regresion test for a bug where `extension`
+// declarations are incorrectly being cached on the declarations
+// they extend, so that an extension of a stdlib type (like `float`)
+// ends up attaching a declaration from one compile request to that
+// type, and then later compile requests that use that stdlib type
+// try to look up through that extension even though (1) that
+// shouldn't make sense semantically, and (2) that extension will
+// have been deallocated when its parent compile request was
+// destroyed.
+//
+// This test relies on the fact that our test runner uses a single
+// slang compilation session (which loads the stdlib code) across
+// multiple compilation tests. We can thus make this file contain
+// two identical tests, with the knowledge that the second one
+// will lead to the bad/crashing behavior if the first one ran
+// and did a Bad Thing.
+
+//TEST:SIMPLE:
+//TEST:SIMPLE:
+
+interface IThing
+{
+	float getThing();
+}
+
+extension float : IThing
+{
+	float getThing() { return this; }
+}
+
+float getThing<T : IThing>(T value)
+{
+	return value.getThing();
+}
+
+float test( float value )
+{
+	return getThing(value);
+}
diff --git a/tests/diagnostics/extension-visibility-a.slang b/tests/diagnostics/extension-visibility-a.slang
new file mode 100644
index 000000000..97e4ef85c
--- /dev/null
+++ b/tests/diagnostics/extension-visibility-a.slang
@@ -0,0 +1,17 @@
+// extension-visibility-a.slang
+
+interface IThing
+{
+	int getValue();
+}
+
+// Note: not implementing the interface here!
+struct MyThing
+{
+	int value;
+}
+
+int helper<T : IThing>(T thing)
+{
+	return thing.getValue();
+}
\ No newline at end of file
diff --git a/tests/diagnostics/extension-visibility-b.slang b/tests/diagnostics/extension-visibility-b.slang
new file mode 100644
index 000000000..7848f2a56
--- /dev/null
+++ b/tests/diagnostics/extension-visibility-b.slang
@@ -0,0 +1,8 @@
+// extension-visibility-b.slang
+
+import extension_visibility_a;
+
+extension MyThing : IThing
+{
+	int getValue() { return value; }	
+}
diff --git a/tests/diagnostics/extension-visibility-c.slang b/tests/diagnostics/extension-visibility-c.slang
new file mode 100644
index 000000000..2d7a5224d
--- /dev/null
+++ b/tests/diagnostics/extension-visibility-c.slang
@@ -0,0 +1,9 @@
+// extension-visibility-c.slang
+
+import extension_visibility_a;
+import extension_visibility_b;
+
+int works(MyThing thing)
+{
+	return helper(thing);
+}
diff --git a/tests/diagnostics/extension-visibility.slang b/tests/diagnostics/extension-visibility.slang
new file mode 100644
index 000000000..029b16b86
--- /dev/null
+++ b/tests/diagnostics/extension-visibility.slang
@@ -0,0 +1,18 @@
+// extension-visibility.slang
+
+// Confirm that visibility of `extensions` is
+// correctly scoped via `import`.
+
+//DIAGNOSTIC_TEST:SIMPLE:
+
+import extension_visibility_a;
+
+// Note: not importing b:
+// import extension_visibility_b;
+
+import extension_visibility_c;
+
+int shouldntWork(MyThing thing)
+{
+	return helper(thing);
+}
diff --git a/tests/diagnostics/extension-visibility.slang.expected b/tests/diagnostics/extension-visibility.slang.expected
new file mode 100644
index 000000000..217dfa188
--- /dev/null
+++ b/tests/diagnostics/extension-visibility.slang.expected
@@ -0,0 +1,7 @@
+result code = -1
+standard error = {
+tests/diagnostics/extension-visibility.slang(17): error 39999: could not specialize generic for arguments of type (MyThing)
+tests/diagnostics/extension-visibility-a.slang(14): note 39999: see declaration of func helper<T>(T) -> int
+}
+standard output = {
+}
diff --git a/tests/language-feature/extensions/extension-import-helper.slang b/tests/language-feature/extensions/extension-import-helper.slang
new file mode 100644
index 000000000..c1a652645
--- /dev/null
+++ b/tests/language-feature/extensions/extension-import-helper.slang
@@ -0,0 +1,13 @@
+// extension-import-helper.slang
+
+//TEST_IGNORE_FILE:
+
+interface IThing
+{
+	float getValue();
+}
+
+extension float : IThing
+{
+	float getValue() { return this; }
+}
\ No newline at end of file
diff --git a/tests/language-feature/extensions/extension-import.slang b/tests/language-feature/extensions/extension-import.slang
new file mode 100644
index 000000000..c5d415a53
--- /dev/null
+++ b/tests/language-feature/extensions/extension-import.slang
@@ -0,0 +1,17 @@
+// extension-import.slang
+
+// Confirm that visibility of extensions through `import` is working.
+
+//TEST:SIMPLE:
+
+import extension_import_helper;
+
+float test<T : IThing>(T value)
+{
+	return value.getValue();
+}
+
+float id(float x)
+{
+	return test(x);
+}
\ No newline at end of file