Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Restricted pattern kinds allowed for a variable definition #1355

Closed
wants to merge 5 commits into from
Closed

Restricted pattern kinds allowed for a variable definition #1355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
45f88ac
Restricted pattern kinds allowed for a variable definition
pk19604014 Jun 30, 2022
ad6bb7f
Merge branch 'trunk' of https://github.com/carbon-language/carbon-lan…
pk19604014 Jul 15, 2022
c698ab4
merged
pk19604014 Aug 12, 2022
1891834
Adding PatternContext to TypeCheckPattern
pk19604014 Aug 12, 2022
528e781
Merge branch 'trunk' of https://github.com/carbon-language/carbon-lan…
pk19604014 Aug 12, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
57 changes: 39 additions & 18 deletions explorer/interpreter/type_checker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2710,8 +2710,8 @@ auto TypeChecker::TypeCheckWhereClause(Nonnull<WhereClause*> clause,

auto TypeChecker::TypeCheckPattern(
Nonnull<Pattern*> p, std::optional<Nonnull<const Value*>> expected,
ImplScope& impl_scope, ValueCategory enclosing_value_category)
-> ErrorOr<Success> {
ImplScope& impl_scope, ValueCategory enclosing_value_category,
PatternContext context) -> ErrorOr<Success> {
if (trace_stream_) {
**trace_stream_ << "checking " << PatternKindName(p->kind()) << " " << *p;
if (expected) {
Expand All @@ -2723,8 +2723,13 @@ auto TypeChecker::TypeCheckPattern(
}
switch (p->kind()) {
case PatternKind::AutoPattern: {
p->set_static_type(arena_->New<TypeType>());
return Success();
if (context.is_binding_pattern_type) {
p->set_static_type(arena_->New<TypeType>());
return Success();
} else {
return CompilationError(p->source_loc())
<< "auto can only appear in the type part of a binding pattern.";
}
}
case PatternKind::BindingPattern: {
auto& binding = cast<BindingPattern>(*p);
Expand All @@ -2735,7 +2740,8 @@ auto TypeChecker::TypeCheckPattern(
<< "The type of a binding pattern cannot contain bindings.";
}
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
&binding.type(), std::nullopt, impl_scope, enclosing_value_category));
&binding.type(), std::nullopt, impl_scope, enclosing_value_category,
context.set_is_binding_pattern_type(true)));
CARBON_ASSIGN_OR_RETURN(
Nonnull<const Value*> type,
InterpPattern(&binding.type(), arena_, trace_stream_));
Expand Down Expand Up @@ -2815,8 +2821,9 @@ auto TypeChecker::TypeCheckPattern(
if (expected) {
expected_field_type = cast<TupleValue>(**expected).elements()[i];
}
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
field, expected_field_type, impl_scope, enclosing_value_category));
CARBON_RETURN_IF_ERROR(
TypeCheckPattern(field, expected_field_type, impl_scope,
enclosing_value_category, context));
if (trace_stream_)
**trace_stream_ << "finished checking tuple pattern field " << *field
<< "\n";
Expand All @@ -2829,6 +2836,10 @@ auto TypeChecker::TypeCheckPattern(
return Success();
}
case PatternKind::AlternativePattern: {
if (!context.refutable_ok) {
return CompilationError(p->source_loc())
<< "Expected an irrefutable pattern, got alternative";
}
auto& alternative = cast<AlternativePattern>(*p);
CARBON_RETURN_IF_ERROR(
TypeCheckExp(&alternative.choice_type(), impl_scope));
Expand All @@ -2852,9 +2863,9 @@ auto TypeChecker::TypeCheckPattern(
<< "'" << alternative.alternative_name()
<< "' is not an alternative of " << choice_type;
}
CARBON_RETURN_IF_ERROR(TypeCheckPattern(&alternative.arguments(),
*parameter_types, impl_scope,
enclosing_value_category));
CARBON_RETURN_IF_ERROR(
TypeCheckPattern(&alternative.arguments(), *parameter_types,
impl_scope, enclosing_value_category, context));
alternative.set_static_type(&choice_type);
CARBON_ASSIGN_OR_RETURN(
Nonnull<const Value*> alternative_value,
Expand All @@ -2869,14 +2880,22 @@ auto TypeChecker::TypeCheckPattern(
CARBON_ASSIGN_OR_RETURN(Nonnull<const Value*> expr_value,
InterpPattern(p, arena_, trace_stream_));
SetValue(p, expr_value);
// TODO
// if (!context.refutable_ok) {
// return CompilationError(p->source_loc())
// << "Expected an irrefutable pattern, got "
// << ExpressionKindName(expression.kind()) << ": " << expression
// << ", expr type: " << expression.static_type()
// << ", expr value: " << *expr_value;
// }
return Success();
}
case PatternKind::VarPattern: {
auto& var_pattern = cast<VarPattern>(*p);

CARBON_RETURN_IF_ERROR(TypeCheckPattern(&var_pattern.pattern(), expected,
impl_scope,
var_pattern.value_category()));
CARBON_RETURN_IF_ERROR(
TypeCheckPattern(&var_pattern.pattern(), expected, impl_scope,
var_pattern.value_category(), context));
var_pattern.set_static_type(&var_pattern.pattern().static_type());
CARBON_ASSIGN_OR_RETURN(
Nonnull<const Value*> pattern_value,
Expand All @@ -2890,9 +2909,9 @@ auto TypeChecker::TypeCheckPattern(
if (expected) {
expected_ptr = arena_->New<PointerType>(expected.value());
}
CARBON_RETURN_IF_ERROR(TypeCheckPattern(&addr_pattern.binding(),
expected_ptr, impl_scope,
enclosing_value_category));
CARBON_RETURN_IF_ERROR(
TypeCheckPattern(&addr_pattern.binding(), expected_ptr, impl_scope,
enclosing_value_category, context));

if (auto* inner_binding_type =
dyn_cast<PointerType>(&addr_pattern.binding().static_type())) {
Expand Down Expand Up @@ -3013,15 +3032,17 @@ auto TypeChecker::TypeCheckStmt(Nonnull<Statement*> s,
// a.(Widget.F)();
// ```
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
&var.pattern(), &rhs_ty, var_scope, var.value_category()));
&var.pattern(), &rhs_ty, var_scope, var.value_category(),
PatternContext().set_refutable_ok(false)));
CARBON_ASSIGN_OR_RETURN(
Nonnull<Expression*> converted_init,
ImplicitlyConvert("initializer of variable", impl_scope,
&var.init(), &var.pattern().static_type()));
var.set_init(converted_init);
} else {
CARBON_RETURN_IF_ERROR(TypeCheckPattern(
&var.pattern(), std::nullopt, var_scope, var.value_category()));
&var.pattern(), std::nullopt, var_scope, var.value_category(),
PatternContext().set_refutable_ok(false)));
}
return Success();
}
Expand Down
21 changes: 20 additions & 1 deletion explorer/interpreter/type_checker.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,24 @@ class TypeChecker {
auto TypeCheckWhereClause(Nonnull<WhereClause*> clause,
const ImplScope& impl_scope) -> ErrorOr<Success>;

// Provides additional context for pattern type checking logic.
struct PatternContext {
PatternContext() : refutable_ok(true), is_binding_pattern_type(false) {}

auto set_refutable_ok(bool b) -> PatternContext {
refutable_ok = b;
return *this;
}

auto set_is_binding_pattern_type(bool b) -> PatternContext {
is_binding_pattern_type = b;
return *this;
}

bool refutable_ok;
bool is_binding_pattern_type;
};

// Equivalent to TypeCheckExp, but operates on the AST rooted at `p`.
//
// `expected` is the type that this pattern is expected to have, if the
Expand All @@ -136,7 +154,8 @@ class TypeChecker {
auto TypeCheckPattern(Nonnull<Pattern*> p,
std::optional<Nonnull<const Value*>> expected,
ImplScope& impl_scope,
ValueCategory enclosing_value_category)
ValueCategory enclosing_value_category,
PatternContext context = PatternContext())
-> ErrorOr<Success>;

// Equivalent to TypeCheckExp, but operates on the AST rooted at `s`.
Expand Down
17 changes: 17 additions & 0 deletions explorer/testdata/basic_syntax/fail_var_pattern.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// 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
//
// RUN: %{not} %{explorer} %s 2>&1 | \
// RUN: %{FileCheck} --match-full-lines --allow-unused-prefixes=false %s
// RUN: %{not} %{explorer} --parser_debug --trace_file=- %s 2>&1 | \
// RUN: %{FileCheck} --match-full-lines --allow-unused-prefixes %s
// AUTOUPDATE: %{explorer} %s

package ExplorerTest api;

fn Main() -> i32 {
// CHECK: COMPILATION ERROR: {{.*}}/explorer/testdata/basic_syntax/fail_var_pattern.carbon:[[@LINE+1]]: auto can only appear in the type part of a binding pattern.
var auto = i32;
return 0;
}
7 changes: 5 additions & 2 deletions explorer/testdata/let/implicit_conversion_choice.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ impl i32 as ImplicitAs(A) {
}

fn Main() -> i32 {
let A.Value(n: i32) = 2;
return n;
let a: A = 2;
match (a) {
case A.Value(n: i32) => { return n; }
}
return 0;
}
13 changes: 2 additions & 11 deletions explorer/testdata/let/nested_tuple_pattern.carbon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,18 +11,9 @@

package ExplorerTest api;

choice Ints {
None,
One(i32),
Two(i32, i32)
}

fn Main() -> i32 {
let (var Ints.Two(a1: auto, var a2: auto),
((b: auto, var c: auto), var (d: auto, e: auto))) =
(Ints.Two(1, 10), ((2, 3), (4, 5)));
a1 = 0;
a2 = 0;
Comment on lines -14 to -25
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test looks at least mostly correct to me (in particular, I don't think let (var Ints.Two(a1: auto, var a2: auto)) is right to just stop testing). Given it's being removed, @geoffromer can you give a look at this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this code is correct, because patterns like Ints.Two(a1: auto, var a2: auto) are refutable, but the pattern in a variable declaration needs to be irrefutable. So I think this code needs to be changed, but I can't speak to whether this change preserves the intent of the test (this is @DarshalShetty's code, not mine).

let (((b: auto, var c: auto), var (d: auto, e: auto))) =
(((2, 3), (4, 5)));
c = 0;
d = 0;
e = 0;
Expand Down