--compare=jit Fails on Functions in Parametric Struct impl Blocks

**Describe the bug**
Consider the following code

```Rust
pub struct Foo {}
pub impl Foo {
fn get_n() -> u32 { N }
}
fn helper() { assert_eq(Foo::get_n(), N); }
#[test]
fn minimal_test() {
helper();
helper();
}
```

breaks with
```
bazel-bin/xls/dslx/interpreter_main --compare=jit test_minimal.x
E0903 20:01:09.197498 351824 command_line_utils.cc:50] Could not extract a textual position from error message: INTERNAL: XLS_RET_CHECK failure (xls/dslx/ir_convert/function_converter.cc:3566) parametric_value.has_value()
=== Source Location Trace: ===
xls/dslx/ir_convert/function_converter.cc:3566
xls/dslx/ir_convert/ir_converter.cc:365
xls/dslx/ir_convert/ir_converter.cc:413
xls/dslx/ir_convert/ir_converter.cc:425
: INVALID_ARGUMENT: Provided status is not in recognized error form: INTERNAL: XLS_RET_CHECK failure (xls/dslx/ir_convert/function_converter.cc:3566) parametric_value.has_value()
=== Source Location Trace: ===
xls/dslx/frontend/bindings.cc:56

Error: INTERNAL: XLS_RET_CHECK failure (xls/dslx/ir_convert/function_converter.cc:3566) parametric_value.has_value() ; Failed to convert input to IR for comparison. Consider turning off comparison with `--compare=none`:
=== Source Location Trace: ===
xls/dslx/ir_convert/function_converter.cc:3566
xls/dslx/ir_convert/ir_converter.cc:365
xls/dslx/ir_convert/ir_converter.cc:413
xls/dslx/ir_convert/ir_converter.cc:425
xls/dslx/run_routines/run_routines.cc:1007
xls/dslx/interpreter_main.cc:280
```

**Expected behavior**
No crash :)

**Environment (this can be helpful for troubleshooting):**
Compiled from head.

**Additional context**
This appears to be because wherever we test for the functions' `IsParametric()` we also should check for ` IsFunctionOnParametricStruct()`

Note all the following was figured out using antigravity which I then guided to make the patch.
So I have no idea what I am doing and leave the actual fix to the maintainers (I find it a bit suspicious that it removed `IsMethodOnParametricStruct()`...)

The following minimal change fixes this particular issue.

```patch
diff --git a/xls/dslx/get_conversion_records.cc b/xls/dslx/get_conversion_records.cc
index 80a883b7d..a49fe0789 100644
--- a/xls/dslx/get_conversion_records.cc
+++ b/xls/dslx/get_conversion_records.cc
@@ -176,7 +176,8 @@ class ConversionRecordVisitor : public AstNodeRecursiveVisitor {
// dealt with in `HandleInvocation`.
absl::Status HandleFunction(const Function* f) override {
VLOG(5) << "HandleFunction " << f->ToString();
- if (f->IsParametric() || f->IsInProc() || f->IsMethodOnParametricStruct()) {
+ if (f->IsParametric() || f->IsInProc() ||
+ f->IsFunctionOnParametricStruct()) {
// TODO: https://github.com/google/xls/issues/1029 - remove module-level
// proc functions.
VLOG(5) << "Skipping function " << f->identifier()
@@ -212,7 +213,7 @@ class ConversionRecordVisitor : public AstNodeRecursiveVisitor {
bool handle_for_invocation) {
XLS_RET_CHECK(module_ == f->owner());
XLS_RET_CHECK(!f->IsInProc());
- if (f->IsParametric() || f->IsMethodOnParametricStruct()) {
+ if (f->IsParametric() || f->IsFunctionOnParametricStruct()) {
XLS_RET_CHECK(!env.empty());
XLS_RET_CHECK_NE(type_info_->GetRoot(), type_info_);
} else {
diff --git a/xls/dslx/ir_convert/extract_conversion_order.cc b/xls/dslx/ir_convert/extract_conversion_order.cc
index 86b0fc814..8b332df14 100644
--- a/xls/dslx/ir_convert/extract_conversion_order.cc
+++ b/xls/dslx/ir_convert/extract_conversion_order.cc
@@ -222,7 +222,8 @@ class InvocationVisitor : public ExprVisitor {
return absl::OkStatus();
}

- bool is_parametric = callee->IsParametric();
+ bool is_parametric = callee->IsParametric() ||
+ callee->IsFunctionOnParametricStruct();
// Temporarily store null type_info for parametric functions. Parametric
// bindings will be resolved below and used to look up the invocation type
// info.
@@ -520,10 +521,13 @@ static void RemoveFunctionDuplicates(std::vector* ready) {
function_cr.f()->tag() == FunctionTag::kProcNext ||
subject_cr.f()->tag() == FunctionTag::kProcConfig ||
subject_cr.f()->tag() == FunctionTag::kProcNext;
+ auto is_parametric_fn = [](Function* f) {
+ return f->IsParametric() || f->IsFunctionOnParametricStruct();
+ };
bool either_is_parametric =
- function_cr.f()->IsParametric() || subject_cr.f()->IsParametric();
+ is_parametric_fn(function_cr.f()) || is_parametric_fn(subject_cr.f());
bool both_are_parametric =
- function_cr.f()->IsParametric() && subject_cr.f()->IsParametric();
+ is_parametric_fn(function_cr.f()) && is_parametric_fn(subject_cr.f());

if (same_fns && !either_is_proc_instance_fn) {
// If neither are parametric, then function identity comparison is
@@ -720,7 +724,8 @@ absl::StatusOr> GetOrder(Module* module,
// NOTE: Proc creation is driven by Spawn instantiations - the
// required constant args are only specified there, so we can't
// convert Procs as encountered at top level.
- if (f->IsParametric() || f->proc().has_value()) {
+ if (f->IsParametric() || f->IsFunctionOnParametricStruct() ||
+ f->proc().has_value()) {
return absl::OkStatus();
}
```

but it is also highlights how error prone the whole situation is, as in every place where we check for a parametric, we have to remember all the parts.

So maybe we should have a helper function in `Function` that does both:

```patch
--- a/xls/dslx/frontend/ast.h
+++ b/xls/dslx/frontend/ast.h
@@ -2606,6 +2606,13 @@ class Function : public AstNode {
// struct.
bool IsFunctionOnParametricStruct() const;

+ // Returns true if this function is parametric itself (has function-level
+ // parametric bindings) or is defined inside an `impl` block of a parametric
+ // struct.
+ bool IsParametricOrOnParametricStruct() const {
+ return IsParametric() || IsFunctionOnParametricStruct();
+ }
+
bool IsStub() const { return is_stub_; }
bool IsCompilerDerived() const { return is_compiler_derived_; }

```

... and then fix all the error prone parts in
* xls/dslx/frontend/ast_utils.cc
* xls/dslx/get_conversion_records.cc
* xls/dslx/ir_convert/extract_conversion_order.cc
* xls/dslx/ir_convert/function_converter.cc

```patch
diff --git a/xls/dslx/frontend/ast_utils.cc b/xls/dslx/frontend/ast_utils.cc
index 890b0442e..ba578447e 100644
--- a/xls/dslx/frontend/ast_utils.cc
+++ b/xls/dslx/frontend/ast_utils.cc
@@ -121,7 +121,7 @@ bool IsParametricFunction(const AstNode* n) {
}

const auto* f = dynamic_cast(n);
- return f != nullptr && f->IsParametric();
+ return f != nullptr && f->IsParametricOrOnParametricStruct();
}

bool IsNameRefToParametricFunction(const AstNode* n) {
diff --git a/xls/dslx/get_conversion_records.cc b/xls/dslx/get_conversion_records.cc
index 80a883b7d..84a3cb7f6 100644
--- a/xls/dslx/get_conversion_records.cc
+++ b/xls/dslx/get_conversion_records.cc
@@ -176,7 +176,7 @@ class ConversionRecordVisitor : public AstNodeRecursiveVisitor {
// dealt with in `HandleInvocation`.
absl::Status HandleFunction(const Function* f) override {
VLOG(5) << "HandleFunction " << f->ToString();
- if (f->IsParametric() || f->IsInProc() || f->IsMethodOnParametricStruct()) {
+ if (f->IsParametricOrOnParametricStruct() || f->IsInProc()) {
// TODO: https://github.com/google/xls/issues/1029 - remove module-level
// proc functions.
VLOG(5) << "Skipping function " << f->identifier()
@@ -212,7 +212,7 @@ class ConversionRecordVisitor : public AstNodeRecursiveVisitor {
bool handle_for_invocation) {
XLS_RET_CHECK(module_ == f->owner());
XLS_RET_CHECK(!f->IsInProc());
- if (f->IsParametric() || f->IsMethodOnParametricStruct()) {
+ if (f->IsParametricOrOnParametricStruct()) {
XLS_RET_CHECK(!env.empty());
XLS_RET_CHECK_NE(type_info_->GetRoot(), type_info_);
} else {
diff --git a/xls/dslx/ir_convert/extract_conversion_order.cc b/xls/dslx/ir_convert/extract_conversion_order.cc
index 86b0fc814..6f8f3ae58 100644
--- a/xls/dslx/ir_convert/extract_conversion_order.cc
+++ b/xls/dslx/ir_convert/extract_conversion_order.cc
@@ -222,7 +222,7 @@ class InvocationVisitor : public ExprVisitor {
return absl::OkStatus();
}

- bool is_parametric = callee->IsParametric();
+ bool is_parametric = callee->IsParametricOrOnParametricStruct();
// Temporarily store null type_info for parametric functions. Parametric
// bindings will be resolved below and used to look up the invocation type
// info.
@@ -521,9 +521,11 @@ static void RemoveFunctionDuplicates(std::vector* ready) {
subject_cr.f()->tag() == FunctionTag::kProcConfig ||
subject_cr.f()->tag() == FunctionTag::kProcNext;
bool either_is_parametric =
- function_cr.f()->IsParametric() || subject_cr.f()->IsParametric();
+ function_cr.f()->IsParametricOrOnParametricStruct() ||
+ subject_cr.f()->IsParametricOrOnParametricStruct();
bool both_are_parametric =
- function_cr.f()->IsParametric() && subject_cr.f()->IsParametric();
+ function_cr.f()->IsParametricOrOnParametricStruct() &&
+ subject_cr.f()->IsParametricOrOnParametricStruct();

if (same_fns && !either_is_proc_instance_fn) {
// If neither are parametric, then function identity comparison is
@@ -720,7 +722,7 @@ absl::StatusOr> GetOrder(Module* module,
// NOTE: Proc creation is driven by Spawn instantiations - the
// required constant args are only specified there, so we can't
// convert Procs as encountered at top level.
- if (f->IsParametric() || f->proc().has_value()) {
+ if (f->IsParametricOrOnParametricStruct() || f->proc().has_value()) {
return absl::OkStatus();
}

diff --git a/xls/dslx/ir_convert/function_converter.cc b/xls/dslx/ir_convert/function_converter.cc
index cfc15c7fc..46d39a8e3 100644
--- a/xls/dslx/ir_convert/function_converter.cc
+++ b/xls/dslx/ir_convert/function_converter.cc
@@ -3637,7 +3637,7 @@ absl::Status FunctionConverter::HandleFunction(
// outside world, since they're driven and named by DSL instantiation, so we
// forgo exposing them here.
if (requires_implicit_token && (node->is_public() || is_top_) &&
- !node->IsParametric() && !node->IsMethodOnParametricStruct()) {
+ !node->IsParametricOrOnParametricStruct()) {
XLS_ASSIGN_OR_RETURN(
xls::Function * wrapper,
EmitImplicitTokenEntryWrapper(ir_fn, node, is_top_,
@@ -4752,7 +4752,7 @@ absl::StatusOr FunctionConverter::GetCalleeIdentifier(
absl::btree_set free_keys = f->GetFreeParametricKeySet();
const CallingConvention convention = GetCallingConvention(f);
Module* m = f->owner();
- if (!f->IsParametric() && !f->IsMethodOnParametricStruct()) {
+ if (!f->IsParametricOrOnParametricStruct()) {
return MangleDslxName(m->name(), f->identifier(), convention, free_keys,
/*parametric_env=*/nullptr, scope);
}
```