Skip to content

Commit

Permalink
Forbid configurables in pattern matching and shadowing (#6289)
Browse files Browse the repository at this point in the history
## Description

This PR:
- emits errors when configurables clash with global constants of the
same name
- forbids shadowing configurables with local constants or variables
- forbids matching against configurables in pattern matching

Before #6058, it was just the flag `is_configurable` in `TyConstantDecl`
that marked constants as configurables. The first two points were
implemented for `TyConstantDecl` and, thus, worked both for constants
and for configurables. When introducing `TyConfigurableDecl` we forgot
to extend those checks to cover the new type declaration.

The third point fixes a bug that allowed matching against configurables,
although they are not compile-time constants.
Originally, matching was done against the configured value, which was
treated as a constant, even if changed during the deployment.

After introducing `TyConfigurableDecl`, because of a bug, configurables
were treated as pattern variable declarations.

This PR brings back the original shadowing behavior and forbids matching
against configurables in pattern matching.

Forbidding configurables in pattern matching can cause confusion,
knowing that regular constants can be matched against. A special
consideration was given to explain to developers why this is the case.

## Breaking Changes

There were two releases in between. Theoretically but unlikely, it could
be that some code appeared that e.g., shadows configurables. In such a
case, the change in this PR will be breaking and cause compiler errors.

## Demo

![Constant of the same name as configurable already
exists](https://github.com/user-attachments/assets/caa84d32-f661-414c-b2e8-869ae2c2d297)

![Configurables cannot be shadowed -
Variable](https://github.com/user-attachments/assets/e95675fa-6754-4e33-bb15-ff923dcb5eba)

![Configurables cannot be matched
against](https://github.com/user-attachments/assets/aebd6462-d254-484b-abaf-45a3ce7ef263)

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [ ] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
  • Loading branch information
ironcev authored Jul 22, 2024
1 parent 0b440ea commit 5085545
Show file tree
Hide file tree
Showing 42 changed files with 1,399 additions and 380 deletions.
1 change: 0 additions & 1 deletion sway-core/src/language/ty/expression/scrutinee.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ pub enum TyScrutineeVariant {
Literal(Literal),
Variable(Ident),
Constant(Ident, Literal, TyConstantDecl),
Configurable(Ident, Literal, TyConfigurableDecl),
StructScrutinee {
struct_ref: DeclRefStruct,
fields: Vec<TyStructScrutineeField>,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,6 @@ pub(crate) fn collect_duplicate_match_pattern_variables(
ty::TyScrutineeVariant::Variable(ident) => add_variable(left_most_branch, ident, false),
ty::TyScrutineeVariant::Literal(_) => (),
ty::TyScrutineeVariant::Constant { .. } => (),
ty::TyScrutineeVariant::Configurable { .. } => (),
ty::TyScrutineeVariant::StructScrutinee { fields, .. } => {
// If a field does not have a scrutinee, the field itself is a variable.
for field in fields {
Expand Down Expand Up @@ -247,3 +246,49 @@ pub(crate) fn collect_duplicate_match_pattern_variables(
}
}
}

/// Returns [Ident]s for all match arm variables found in the `scrutinee`,
/// together with the information if the variable is a struct field (true)
/// or not (false), or empty [Vec] if there are no variables declared in
/// the `scrutinee`.
///
/// If the `scrutinee` contains alternatives, and thus a variable is declared
/// multiple times, each occurrence of the variable will be returned.
pub(crate) fn collect_match_pattern_variables(scrutinee: &TyScrutinee) -> Vec<(Ident, bool)> {
let mut variables = vec![];

recursively_collect_variables(&mut variables, scrutinee);

return variables;

fn recursively_collect_variables(variables: &mut Vec<(Ident, bool)>, scrutinee: &TyScrutinee) {
match &scrutinee.variant {
ty::TyScrutineeVariant::CatchAll => (),
ty::TyScrutineeVariant::Variable(ident) => variables.push((ident.clone(), false)),
ty::TyScrutineeVariant::Literal(_) => (),
ty::TyScrutineeVariant::Constant { .. } => (),
ty::TyScrutineeVariant::StructScrutinee { fields, .. } => {
// If a field does not have a scrutinee, the field itself is a variable.
for field in fields {
match &field.scrutinee {
Some(scrutinee) => recursively_collect_variables(variables, scrutinee),
None => variables.push((field.field.clone(), true)),
}
}
}
ty::TyScrutineeVariant::Or(scrutinees) => {
for scrutinee in scrutinees {
recursively_collect_variables(variables, scrutinee);
}
}
ty::TyScrutineeVariant::Tuple(scrutinees) => {
for scrutinee in scrutinees {
recursively_collect_variables(variables, scrutinee);
}
}
ty::TyScrutineeVariant::EnumScrutinee { value, .. } => {
recursively_collect_variables(variables, value)
}
}
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
mod constructor_factory;
mod duplicates;
mod match_pattern_variables;
mod matrix;
mod patstack;
mod pattern;
Expand All @@ -8,6 +8,8 @@ mod reachable_report;
mod usefulness;
mod witness_report;

pub(crate) use duplicates::collect_duplicate_match_pattern_variables;
pub(crate) use match_pattern_variables::{
collect_duplicate_match_pattern_variables, collect_match_pattern_variables,
};
pub(in crate::semantic_analysis::ast_node::expression) use reachable_report::ReachableReport;
pub(crate) use usefulness::check_match_expression_usefulness;
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,6 @@ impl Pattern {
ty::TyScrutineeVariant::Variable(_) => Pattern::Wildcard,
ty::TyScrutineeVariant::Literal(value) => Pattern::from_literal(value),
ty::TyScrutineeVariant::Constant(_, value, _) => Pattern::from_literal(value),
ty::TyScrutineeVariant::Configurable(_, value, _) => Pattern::from_literal(value),
ty::TyScrutineeVariant::StructScrutinee {
struct_ref,
fields,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use indexmap::IndexMap;

use crate::{
language::{
ty::{self, TyConfigurableDecl, TyConstantDecl},
ty::{self, TyConstantDecl},
CallPath, Literal,
},
semantic_analysis::{
Expand Down Expand Up @@ -229,9 +229,6 @@ pub(super) fn matcher(
ty::TyScrutineeVariant::Constant(_, _, const_decl) => {
Ok(match_constant(ctx, exp, const_decl, span))
}
ty::TyScrutineeVariant::Configurable(_, _, config_decl) => {
Ok(match_configurable(ctx, exp, config_decl, span))
}
ty::TyScrutineeVariant::StructScrutinee {
struct_ref: _,
fields,
Expand Down Expand Up @@ -422,31 +419,6 @@ fn match_constant(
ReqDeclTree::req(req)
}

fn match_configurable(
ctx: TypeCheckContext,
exp: &ty::TyExpression,
decl: TyConfigurableDecl,
span: Span,
) -> ReqDeclTree {
let name = decl.name().clone();
let return_type = decl.type_ascription.type_id;

let req = (
exp.to_owned(),
ty::TyExpression {
expression: ty::TyExpressionVariant::ConfigurableExpression {
span: span.clone(),
decl: Box::new(decl),
call_path: Some(CallPath::from(name).to_fullpath(ctx.engines(), ctx.namespace())),
},
return_type,
span,
},
);

ReqDeclTree::req(req)
}

fn match_struct(
handler: &Handler,
mut ctx: TypeCheckContext,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use ast_node::expression::match_expression::typed::matcher::{ReqDeclNode, ReqOrVarDecl};
use indexmap::IndexSet;
use itertools::{multiunzip, Itertools};
use sway_error::{
error::CompileError,
error::{CompileError, ShadowingSource},
handler::{ErrorEmitted, Handler},
};
use sway_types::{Ident, Span, Spanned};
Expand Down Expand Up @@ -43,10 +44,10 @@ impl ty::TyMatchBranch {
let type_engine = ctx.engines.te();
let engines = ctx.engines();

// type check the scrutinee
// Type check the scrutinee.
let typed_scrutinee = ty::TyScrutinee::type_check(handler, ctx.by_ref(), scrutinee)?;

// calculate the requirements and variable declarations
// Calculate the requirements and variable declarations.
let req_decl_tree = matcher(
handler,
ctx.by_ref(),
Expand Down Expand Up @@ -76,6 +77,48 @@ impl ty::TyMatchBranch {
Ok(())
})?;

// Emit errors for eventual uses of configurables in patterns.
// Configurables cannot be used in pattern matching, since they are not compile-time
// constants. Using a configurable will define a pattern variable, that will
// then shadow the configurable of the same name, which is not allowed.
// This can be very confusing in case someone tries to use configurables in pattern matching
// in the same way as constants. So we provide helpful hints here.
// We stop further compilation in case of finding configurables in patterns.
handler.scope(|handler| {
// All the first occurrences of variables in order of appearance, while respecting
// if they are struct field variables.
let variables: IndexSet<(Ident, bool)> =
IndexSet::from_iter(collect_match_pattern_variables(&typed_scrutinee));
for (ident, is_struct_field) in variables {
let default_handler = &Handler::default();
// If there exist a configurable with the same name as the pattern variable.
if let Ok(ty::TyDecl::ConfigurableDecl(configurable_decl)) = ctx
.namespace()
.resolve_symbol_typed(default_handler, engines, &ident, ctx.self_type())
{
let name = (&ident).into();
let configurable_span = engines
.de()
.get_configurable(&configurable_decl.decl_id)
.span();
if is_struct_field {
handler.emit_err(CompileError::ConfigurablesCannotBeShadowed {
shadowing_source: ShadowingSource::PatternMatchingStructFieldVar,
name,
configurable_span,
});
} else {
handler.emit_err(CompileError::ConfigurablesCannotBeMatchedAgainst {
name,
configurable_span,
});
}
}
}

Ok(())
})?;

let (condition, result_var_declarations, or_variant_vars) =
instantiate_branch_condition_result_var_declarations_and_matched_or_variant_index_vars(
handler,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,6 @@ impl TyScrutinee {
ty::TyScrutineeVariant::Variable(_) => true,
ty::TyScrutineeVariant::Literal(_) => false,
ty::TyScrutineeVariant::Constant { .. } => false,
ty::TyScrutineeVariant::Configurable { .. } => false,
ty::TyScrutineeVariant::StructScrutinee { fields, .. } => fields
.iter()
.filter_map(|x| x.scrutinee.as_ref())
Expand All @@ -155,6 +154,8 @@ impl TyScrutinee {
}
}

/// Type checks the `name`, assuming that it's either a variable or an ambiguous identifier
/// that might be a constant or configurable.
fn type_check_variable(
handler: &Handler,
ctx: TypeCheckContext,
Expand All @@ -170,14 +171,14 @@ fn type_check_variable(
.resolve_symbol_typed(&Handler::default(), engines, &name, ctx.self_type())
.ok()
{
// If this variable is a constant, then we turn it into a [TyScrutinee::Constant](ty::TyScrutinee::Constant).
// If the name represents a constant, then we turn it into a [ty::TyScrutineeVariant::Constant].
Some(ty::TyDecl::ConstantDecl(ty::ConstantDecl { decl_id, .. })) => {
let constant_decl = (*decl_engine.get_constant(&decl_id)).clone();
let value = match constant_decl.value {
Some(ref value) => value,
None => {
return Err(handler.emit_err(CompileError::Internal(
"constant value does not contain expression",
"Constant value does not contain expression",
span,
)));
}
Expand All @@ -198,7 +199,15 @@ fn type_check_variable(
span,
}
}
// Variable isn't a constant, so we turn it into a [ty::TyScrutinee::Variable].
// If the name isn't a constant, we turn it into a [ty::TyScrutineeVariant::Variable].
//
// Note that the declaration could be a configurable declaration, [ty::ConfigurableDecl].
// Configurables cannot be matched against, but we do not emit that error here.
// That would unnecessary short-circuit the compilation and reduce number of errors
// collected.
// Rather, we consider the configurable to be a pattern variable declaration, which
// strictly speaking it is. Later when checking typed match arm, we will emit
// appropriate helpful errors, depending on the exact usage of that configurable.
_ => ty::TyScrutinee {
variant: ty::TyScrutineeVariant::Variable(name),
type_id: type_engine.insert(ctx.engines(), TypeInfo::Unknown, None),
Expand Down
Loading

0 comments on commit 5085545

Please sign in to comment.