Skip to content

Commit

Permalink
[DSLX:TS] Fix TypeMissingInfo ICE where a builtin parametric fn wasn'…
Browse files Browse the repository at this point in the history
…t invoked.

Found via coverage-guided fuzzing.

PiperOrigin-RevId: 596748305
  • Loading branch information
cdleary authored and copybara-github committed Jan 9, 2024
1 parent ffc73fc commit a8bd634
Show file tree
Hide file tree
Showing 12 changed files with 46 additions and 2 deletions.
2 changes: 2 additions & 0 deletions xls/dslx/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -352,10 +352,12 @@ cc_library(
srcs = ["dslx_builtins.cc"],
hdrs = ["dslx_builtins.h"],
deps = [
":channel_direction",
":errors",
":interp_value",
"//xls/common/logging",
"//xls/common/status:status_macros",
"//xls/dslx/frontend:ast",
"//xls/dslx/frontend:ast_node",
"//xls/dslx/frontend:builtins_metadata",
"//xls/dslx/frontend:pos",
Expand Down
3 changes: 2 additions & 1 deletion xls/dslx/dslx_builtins.cc
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
#include <string>
#include <string_view>
#include <utility>
#include <variant>
#include <vector>

#include "absl/container/flat_hash_map.h"
Expand All @@ -33,7 +32,9 @@
#include "absl/types/span.h"
#include "xls/common/logging/logging.h"
#include "xls/common/status/status_macros.h"
#include "xls/dslx/channel_direction.h"
#include "xls/dslx/errors.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/frontend/ast_node.h"
#include "xls/dslx/frontend/builtins_metadata.h"
#include "xls/dslx/frontend/pos.h"
Expand Down
7 changes: 7 additions & 0 deletions xls/dslx/dslx_builtins.h
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@

#include "absl/container/flat_hash_set.h"
#include "absl/status/statusor.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/frontend/pos.h"
#include "xls/dslx/interp_value.h"
#include "xls/dslx/type_system/concrete_type.h"
#include "xls/dslx/type_system/deduce_ctx.h"
#include "xls/dslx/type_system/parametric_constraint.h"
Expand Down Expand Up @@ -81,6 +84,10 @@ using SignatureFn = std::function<absl::StatusOr<TypeAndParametricEnv>(
absl::StatusOr<SignatureFn> GetParametricBuiltinSignature(
std::string_view builtin_name);

// Wrapper around the above that checks whether name_ref refers to a builtin
// name def and whether that builtin name is a parametric function.
bool IsBuiltinParametricNameRef(const NameRef* name_ref);

} // namespace xls::dslx

#endif // XLS_DSLX_DSLX_BUILTINS_H_
3 changes: 3 additions & 0 deletions xls/dslx/frontend/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ cc_library(
"//xls/common/status:ret_check",
"//xls/common/status:status_builder",
"//xls/common/status:status_macros",
"//xls/dslx:channel_direction",
"//xls/ir:code_template",
"//xls/ir:foreign_function",
"//xls/ir:format_strings",
Expand Down Expand Up @@ -163,6 +164,7 @@ cc_library(
"//xls/common/status:status_macros",
"//xls/dslx:import_data",
"//xls/dslx:interp_value",
"//xls/dslx/frontend:builtins_metadata",
"//xls/dslx/type_system:type_info",
],
)
Expand Down Expand Up @@ -286,6 +288,7 @@ cc_library(
hdrs = ["bindings.h"],
deps = [
":ast",
":pos",
"@com_google_absl//absl/container:flat_hash_map",
"@com_google_absl//absl/container:flat_hash_set",
"@com_google_absl//absl/status",
Expand Down
10 changes: 10 additions & 0 deletions xls/dslx/frontend/ast_utils.cc
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,10 @@
#include "xls/common/status/status_macros.h"
#include "xls/common/visitor.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/frontend/builtins_metadata.h"
#include "xls/dslx/frontend/token_utils.h"
#include "xls/dslx/import_data.h"
#include "xls/dslx/interp_value.h"
#include "xls/dslx/type_system/type_info.h"

namespace xls::dslx {
Expand Down Expand Up @@ -651,4 +653,12 @@ absl::StatusOr<std::vector<const NameDef*>> CollectReferencedUnder(
return name_defs;
}

bool IsBuiltinParametricNameRef(const NameRef* name_ref) {
// Implementation note: we also check IsNameParametricBuiltin() as future
// proofing -- we may add a built-in name that is not a type or parametric
// function.
return std::holds_alternative<BuiltinNameDef*>(name_ref->name_def()) &&
IsNameParametricBuiltin(name_ref->identifier());
}

} // namespace xls::dslx
4 changes: 4 additions & 0 deletions xls/dslx/frontend/ast_utils.h
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,10 @@ absl::StatusOr<std::vector<const AstNode*>> CollectUnder(const AstNode* root,
absl::StatusOr<std::vector<const NameDef*>> CollectReferencedUnder(
const AstNode* root, bool want_types = false);

// Wrapper around the above that checks whether name_ref refers to a builtin
// name def and whether that builtin name is a parametric function.
bool IsBuiltinParametricNameRef(const NameRef* name_ref);

} // namespace xls::dslx

#endif // XLS_DSLX_FRONTEND_AST_UTILS_H_
1 change: 1 addition & 0 deletions xls/dslx/frontend/bindings.h
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
#include "absl/strings/str_format.h"
#include "xls/common/logging/logging.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/frontend/pos.h"

namespace xls::dslx {

Expand Down
2 changes: 2 additions & 0 deletions xls/dslx/frontend/parser.cc
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <cstddef>
#include <cstdint>
#include <functional>
#include <initializer_list>
#include <memory>
#include <optional>
#include <string>
Expand All @@ -43,6 +44,7 @@
#include "xls/common/status/status_builder.h"
#include "xls/common/status/status_macros.h"
#include "xls/common/visitor.h"
#include "xls/dslx/channel_direction.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/frontend/ast_cloner.h"
#include "xls/dslx/frontend/ast_utils.h"
Expand Down
1 change: 1 addition & 0 deletions xls/dslx/type_system/BUILD
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,7 @@ cc_library(
"//xls/ir:bits",
"//xls/ir:bits_ops",
"//xls/ir:format_preference",
"//xls/ir:format_strings",
],
)

Expand Down
1 change: 1 addition & 0 deletions xls/dslx/type_system/concrete_type.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@
#include "absl/status/status.h"
#include "absl/status/statusor.h"
#include "absl/types/span.h"
#include "xls/common/logging/logging.h"
#include "xls/dslx/channel_direction.h"
#include "xls/dslx/frontend/ast.h"
#include "xls/dslx/interp_value.h"
Expand Down
4 changes: 3 additions & 1 deletion xls/dslx/type_system/deduce.cc
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
#include "xls/ir/bits.h"
#include "xls/ir/bits_ops.h"
#include "xls/ir/format_preference.h"
#include "xls/ir/format_strings.h"

namespace xls::dslx {
namespace {
Expand Down Expand Up @@ -3239,7 +3240,8 @@ absl::StatusOr<std::unique_ptr<ConcreteType>> DeduceNameRef(const NameRef* node,
// If this has no corresponding type because it is a parametric function that
// is not being invoked, we give an error instead of propagating
// "TypeMissing".
if (IsParametricFunction(node->GetDefiner()) &&
if ((IsParametricFunction(node->GetDefiner()) ||
IsBuiltinParametricNameRef(node)) &&
!ParentIsInvocationWithCallee(node)) {
return TypeInferenceErrorStatus(
node->span(), nullptr,
Expand Down
10 changes: 10 additions & 0 deletions xls/dslx/type_system/typecheck_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2204,5 +2204,15 @@ fn f(x: Point1D) -> Point1D { x }
HasSubstr("Parametric type being returned from function")));
}

TEST(TypecheckErrorTest, OperatorOnParametricBuiltin) {
EXPECT_THAT(Typecheck(R"(
fn f() { fail!%2 }
)")
.status(),
IsPosError("TypeInferenceError",
HasSubstr("Name 'fail!' is a parametric function, but "
"it is not being invoked")));
}

} // namespace
} // namespace xls::dslx

0 comments on commit a8bd634

Please sign in to comment.