carbon-language/carbon-lang
1
0
Fork 0
mirror of https://github.com/carbon-language/carbon-lang.git synced 2026-10-05 22:02:55 +01:00
Code Issues Packages Projects Releases 10 Wiki Activity

Explorer: Include self pattern in method type (#2935)

Browse Source 
This fixes a bug where Explorer would not detect when:

- an implementation used a method to implement a class function in an interface
- an implementation used a class function to implement a method in an interface
- an implementation method used `addr self` when the interface method did not
- an interface method used `addr self` when the implementation method did not

at type checking time. This would then cause a crash at runtime.

Closes #2857

---------

Co-authored-by: Richard Smith <richard@metafoo.co.uk>
This commit is contained in:
josh11b
2023-06-28 09:07:01 -07:00
committed by GitHub
co-authored by Richard Smith
parent 594ebff23b
commit 318eb793eb
10 changed files with 213 additions and 19 deletions
Download Patch File Download Diff File Expand all files Collapse all files
explorer/ast/value.cpp
+22 -1
View File
@@ -585,13 +585,21 @@ void Value::Print(llvm::raw_ostream& out) const {
case Value::Kind::FunctionType: {
const auto& fn_type = cast<FunctionType>(*this);
out << "fn ";
if (!fn_type.deduced_bindings().empty()) {
auto self = fn_type.method_self();
if (!fn_type.deduced_bindings().empty() || self.has_value()) {
out << "[";
llvm::ListSeparator sep;
for (Nonnull<const GenericBinding*> deduced :
fn_type.deduced_bindings()) {
out << sep << *deduced;
}
if (self.has_value()) {
if (self->addr_self) {
out << sep << "addr self: " << *self->self_type << "*";
} else {
out << sep << "self: " << *self->self_type;
}
}
out << "]";
}
out << fn_type.parameters() << " -> " << fn_type.return_type();
@@ -844,6 +852,19 @@ auto TypeEqual(Nonnull<const Value*> t1, Nonnull<const Value*> t2,
case Value::Kind::FunctionType: {
const auto& fn1 = cast<FunctionType>(*t1);
const auto& fn2 = cast<FunctionType>(*t2);
// Verify `self` parameters match
auto self1 = fn1.method_self();
auto self2 = fn2.method_self();
if (self1.has_value() != self2.has_value()) {
return false;
}
if (self1) {
if (self1->addr_self != self2->addr_self ||
!TypeEqual(self1->self_type, self2->self_type, equality_ctx)) {
return false;
}
}
// Verify parameters and return types match
return TypeEqual(&fn1.parameters(), &fn2.parameters(), equality_ctx) &&
TypeEqual(&fn1.return_type(), &fn2.return_type(), equality_ctx);
}
explorer/ast/value.h
+31 -5
View File
@@ -662,18 +662,34 @@ class FunctionType : public Value {
Nonnull<const GenericBinding*> binding;
};
FunctionType(Nonnull<const Value*> parameters,
// For methods with unbound `self` parameters.
struct MethodSelf {
template <typename F>
auto Decompose(F f) const {
return f(addr_self, self_type);
}
// True if `self` parameter uses an `addr` pattern.
bool addr_self;
// Type of `self` parameter.
const Value* self_type;
};
FunctionType(std::optional<MethodSelf> method_self,
Nonnull<const Value*> parameters,
Nonnull<const Value*> return_type)
: FunctionType(parameters, {}, return_type, {}, {},
: FunctionType(method_self, parameters, {}, return_type, {}, {},
/*is_initializing=*/false) {}
FunctionType(Nonnull<const Value*> parameters,
FunctionType(std::optional<MethodSelf> method_self,
Nonnull<const Value*> parameters,
std::vector<GenericParameter> generic_parameters,
Nonnull<const Value*> return_type,
std::vector<Nonnull<const GenericBinding*>> deduced_bindings,
std::vector<Nonnull<const ImplBinding*>> impl_bindings,
bool is_initializing)
: Value(Kind::FunctionType),
method_self_(method_self),
parameters_(parameters),
generic_parameters_(std::move(generic_parameters)),
return_type_(return_type),
@@ -681,14 +697,20 @@ class FunctionType : public Value {
impl_bindings_(std::move(impl_bindings)),
is_initializing_(is_initializing) {}
struct ExceptSelf {};
FunctionType(ExceptSelf, const FunctionType& clone)
: FunctionType(std::nullopt, clone.parameters_, clone.generic_parameters_,
clone.return_type_, clone.deduced_bindings_,
clone.impl_bindings_, clone.is_initializing_) {}
static auto classof(const Value* value) -> bool {
return value->kind() == Kind::FunctionType;
}
template <typename F>
auto Decompose(F f) const {
return f(parameters_, generic_parameters_, return_type_, deduced_bindings_,
impl_bindings_, is_initializing_);
return f(method_self_, parameters_, generic_parameters_, return_type_,
deduced_bindings_, impl_bindings_, is_initializing_);
}
// The type of the function parameter tuple.
@@ -713,7 +735,11 @@ class FunctionType : public Value {
// Return whether the function type is an initializing expression or not.
auto is_initializing() const -> bool { return is_initializing_; }
// Binding for the implicit `self` parameter, if this is an unbound method.
auto method_self() const -> std::optional<MethodSelf> { return method_self_; }
private:
std::optional<MethodSelf> method_self_;
Nonnull<const Value*> parameters_;
std::vector<GenericParameter> generic_parameters_;
Nonnull<const Value*> return_type_;
explorer/interpreter/type_checker.cpp
+59 -10
View File
@@ -2430,8 +2430,18 @@ class TypeChecker::SubstituteTransform
CARBON_ASSIGN_OR_RETURN(const auto* ret, type_checker_->SubstituteImpl(
subst_bindings.bindings(),
&fn_type->return_type()));
std::optional<FunctionType::MethodSelf> method_self =
fn_type->method_self();
if (method_self.has_value()) {
CARBON_ASSIGN_OR_RETURN(
const auto* self_type,
type_checker_->SubstituteImpl(subst_bindings.bindings(),
method_self->self_type));
method_self->self_type = self_type;
}
return type_checker_->arena_->New<FunctionType>(
param, std::move(generic_parameters), ret, std::move(deduced_bindings),
method_self, param, std::move(generic_parameters), ret,
std::move(deduced_bindings),
std::move(subst_bindings).TakeImplBindings(),
fn_type->is_initializing());
}
@@ -3082,10 +3092,10 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
Nonnull<const Value*> field_type,
Substitute(member_t_class->bindings(), member_type));
access.set_member(arena_->New<NamedElement>(member));
access.set_static_type(field_type);
access.set_is_type_access(!IsInstanceMember(&access.member()));
switch (member->kind()) {
case DeclarationKind::VariableDeclaration:
access.set_static_type(field_type);
access.set_expression_category(
access.object().expression_category());
break;
@@ -3093,6 +3103,14 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
const auto* func_decl = cast<FunctionDeclaration>(member);
CARBON_RETURN_IF_ERROR(CheckAddrMeAccess(
&access, func_decl, t_class.bindings(), impl_scope));
if (access.is_type_access()) {
access.set_static_type(field_type);
} else {
// Remove `self` from type since now bound.
auto* function_type = cast<FunctionType>(field_type);
access.set_static_type(arena_->New<FunctionType>(
FunctionType::ExceptSelf{}, *function_type));
}
access.set_expression_category(ExpressionCategory::Value);
break;
}
@@ -3162,13 +3180,22 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
access.set_member(arena_->New<NamedElement>(result.member));
access.set_found_in_interface(result.interface);
access.set_is_type_access(!IsInstanceMember(&access.member()));
access.set_static_type(inst_member_type);
access.set_expression_category(ExpressionCategory::Value);
if (const auto* func_decl =
dyn_cast<FunctionDeclaration>(result.member)) {
CARBON_RETURN_IF_ERROR(
CheckAddrMeAccess(&access, func_decl, bindings, impl_scope));
if (access.is_type_access()) {
access.set_static_type(inst_member_type);
} else {
// Remove `self` from type since now bound.
auto* function_type = cast<FunctionType>(inst_member_type);
access.set_static_type(arena_->New<FunctionType>(
FunctionType::ExceptSelf{}, *function_type));
}
} else {
access.set_static_type(inst_member_type);
}
// TODO: This is just a ConstraintImplWitness into the
@@ -3286,8 +3313,8 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
Nonnull<const Value*> parameter_type,
Substitute(choice.bindings(),
*(*signature)->parameters_static_type()));
Nonnull<const Value*> type =
arena_->New<FunctionType>(parameter_type, &choice);
Nonnull<const Value*> type = arena_->New<FunctionType>(
std::nullopt, parameter_type, &choice);
// TODO: Should there be a Declaration corresponding to each
// choice type alternative?
access.set_member(
@@ -3465,6 +3492,16 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
return Success();
};
auto set_static_type_remove_self = [&]() -> ErrorOr<Success> {
Nonnull<const Value*> member_type = &member_name.member().type();
CARBON_ASSIGN_OR_RETURN(member_type,
Substitute(bindings_for_member(), member_type));
auto* function_type = cast<FunctionType>(member_type);
access.set_static_type(arena_->New<FunctionType>(
FunctionType::ExceptSelf{}, *function_type));
return Success();
};
switch (std::optional<Nonnull<const Declaration*>> decl =
member_name.member().declaration();
decl ? decl.value()->kind()
@@ -3484,7 +3521,12 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
CARBON_CHECK(!has_instance || is_instance_member ||
!member_name.base_type().has_value())
<< "vacuous compound member access";
CARBON_RETURN_IF_ERROR(set_static_type_as_member_type());
// If this is instance access, remove self bound from function type
if (has_instance && is_instance_member) {
CARBON_RETURN_IF_ERROR(set_static_type_remove_self());
} else {
CARBON_RETURN_IF_ERROR(set_static_type_as_member_type());
}
access.set_expression_category(ExpressionCategory::Value);
CARBON_RETURN_IF_ERROR(
CheckAddrMeAccess(&access, cast<FunctionDeclaration>(*decl),
@@ -3849,7 +3891,8 @@ auto TypeChecker::TypeCheckExpImpl(Nonnull<Expression*> e,
TypeCheckTypeExp(&fn.return_type(), impl_scope));
fn.set_static_type(arena_->New<TypeType>());
fn.set_expression_category(ExpressionCategory::Value);
fn.set_constant_value(arena_->New<FunctionType>(param, ret));
fn.set_constant_value(
arena_->New<FunctionType>(std::nullopt, param, ret));
return Success();
}
case ExpressionKind::StringLiteral:
@@ -5061,12 +5104,17 @@ auto TypeChecker::DeclareCallableDeclaration(Nonnull<CallableDeclaration*> f,
CollectImplBindingsInPattern(deduced, impl_bindings);
}
// Type check the receiver pattern.
std::optional<FunctionType::MethodSelf> method_self;
if (f->is_method()) {
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
&f->self_pattern(), PatternRequirements::Irrefutable, std::nullopt,
function_scope, ExpressionCategory::Value));
CollectAndNumberGenericBindingsInPattern(&f->self_pattern(), all_bindings);
CollectImplBindingsInPattern(&f->self_pattern(), impl_bindings);
FunctionType::MethodSelf method_self_present = {
(f->self_pattern().kind() == PatternKind::AddrPattern),
&f->self_pattern().static_type()};
method_self = method_self_present;
}
// Type check the parameter pattern.
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
@@ -5124,9 +5172,10 @@ auto TypeChecker::DeclareCallableDeclaration(Nonnull<CallableDeclaration*> f,
CARBON_CHECK(IsNonDeduceableType(&f->return_term().static_type()));
f->set_static_type(arena_->New<FunctionType>(
&f->param_pattern().static_type(), std::move(generic_parameters),
&f->return_term().static_type(), std::move(deduced_bindings),
std::move(impl_bindings), /*is_initializing*/ true));
method_self, &f->param_pattern().static_type(),
std::move(generic_parameters), &f->return_term().static_type(),
std::move(deduced_bindings), std::move(impl_bindings),
/*is_initializing*/ true));
switch (f->kind()) {
case DeclarationKind::FunctionDeclaration:
// TODO: Should we pass in the bindings from the enclosing scope?
explorer/interpreter/type_structure.cpp
+1
View File
@@ -81,6 +81,7 @@ struct TypeStructureBuilder {
void Visit(const Address&) {}
void Visit(const VTable&) {}
void Visit(const FunctionType::GenericParameter&) {}
void Visit(const FunctionType::MethodSelf&) {}
void Visit(const NamedElement&) {}
// Constraint types can contain mentions of VariableTypes, but they aren't
explorer/testdata/class/bound_method_compound_member_access.carbon
Vendored
+25
View File
@@ -0,0 +1,25 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
// AUTOUPDATE
// CHECK:STDOUT: result: 0
package ExplorerTest api;
class Point {
fn Origin() -> Point {
return {.x = 0, .y = 0};
}
fn GetX[self: Point]() -> i32 {
return self.x;
}
var x: i32;
var y: i32;
}
fn Main() -> i32 {
var p: Point = Point.Origin();
var f: __Fn() -> i32 = p.(Point.GetX);
return f();
}
explorer/testdata/interface/fail_impl_adds_self.carbon
Vendored
+24
View File
@@ -0,0 +1,24 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
// AUTOUPDATE
package ExplorerTest api;
interface I {
fn G();
}
class C {
impl as I {
// CHECK:STDERR: COMPILATION ERROR: fail_impl_adds_self.carbon:[[@LINE+3]]: type error in member of implementation
// CHECK:STDERR: expected: fn () -> ()
// CHECK:STDERR: actual: fn [self: class C]() -> ()
fn G[self: Self]() { }
}
}
fn Main() -> i32 {
return 0;
}
explorer/testdata/interface/fail_impl_bad_member.carbon
Vendored
+2 -2
View File
@@ -21,8 +21,8 @@ class Point {
fn Scale[self: Point](v: i32) -> i32 {
return 0;
// CHECK:STDERR: COMPILATION ERROR: fail_impl_bad_member.carbon:[[@LINE+3]]: type error in member of implementation
// CHECK:STDERR: expected: fn (i32,) -> class Point
// CHECK:STDERR: actual: fn (i32,) -> i32
// CHECK:STDERR: expected: fn [self: class Point](i32,) -> class Point
// CHECK:STDERR: actual: fn [self: class Point](i32,) -> i32
}
}
}
explorer/testdata/interface/fail_impl_mismatch_self.carbon
Vendored
+24
View File
@@ -0,0 +1,24 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
// AUTOUPDATE
package ExplorerTest api;
interface I {
fn G[self: Self]();
}
class C {
impl as I {
// CHECK:STDERR: COMPILATION ERROR: fail_impl_mismatch_self.carbon:[[@LINE+3]]: type error in member of implementation
// CHECK:STDERR: expected: fn [self: class C]() -> ()
// CHECK:STDERR: actual: fn [addr self: class C*]() -> ()
fn G[addr self: Self*]() { }
}
}
fn Main() -> i32 {
return 0;
}
explorer/testdata/interface/fail_impl_missing_self.carbon
Vendored
+24
View File
@@ -0,0 +1,24 @@
// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
// Exceptions. See /LICENSE for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
//
// AUTOUPDATE
package ExplorerTest api;
interface I {
fn G[self: Self]();
}
class C {
impl as I {
// CHECK:STDERR: COMPILATION ERROR: fail_impl_missing_self.carbon:[[@LINE+3]]: type error in member of implementation
// CHECK:STDERR: expected: fn [self: class C]() -> ()
// CHECK:STDERR: actual: fn () -> ()
fn G() { }
}
}
fn Main() -> i32 {
return 0;
}
explorer/testdata/member_access/fail_qualified_non_member.carbon
Vendored
+1 -1
View File
@@ -9,7 +9,7 @@ fn F[self: i32]() {}
fn Main() -> i32 {
// TODO: It's unclear whether this is valid per the current rules. See
// https://github.com/carbon-language/carbon-lang/pull/1122
// CHECK:STDERR: COMPILATION ERROR: fail_qualified_non_member.carbon:[[@LINE+1]]: expected name of instance member or interface member in compound member access, found fn () -> ()
// CHECK:STDERR: COMPILATION ERROR: fail_qualified_non_member.carbon:[[@LINE+1]]: expected name of instance member or interface member in compound member access, found fn [self: i32]() -> ()
42.(F)();
return 0;
}