Skip to content

Commit

Permalink
Auto merge of #14932 - HKalbasi:dev, r=HKalbasi
Browse files Browse the repository at this point in the history
Lower const params with a bad id

cc #7434

This PR adds an `InTypeConstId` which is a `DefWithBodyId` and lower const generic parameters into bodies using it, and evaluate them with the mir interpreter. I think this is the last unimplemented const generic feature relative to rustc stable.

But there is a problem: The id used in the `InTypeConstId` is the raw `FileAstId`, which changes frequently. So these ids and their bodies will be invalidated very frequently, which is bad for incremental analysis.

Due this problem, I disabled lowering for local crates (in library crate the id is stable since files won't be changed). This might be overreacting (const generic expressions are usually small, maybe it would be better enabled with bad performance than disabled) but it makes motivation for doing it in the correct way, and it splits the potential panic and breakages that usually comes with const generic PRs in two steps.

Other than the id, I think (at least I hope) other parts are in the right direction.
  • Loading branch information
bors committed Jun 12, 2023
2 parents 38c47df + a469578 commit dcd3155
Show file tree
Hide file tree
Showing 48 changed files with 716 additions and 214 deletions.
6 changes: 3 additions & 3 deletions crates/base-db/src/fixture.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ impl ChangeFixture {
None,
default_cfg,
Default::default(),
Env::default(),
Env::new_for_test_fixture(),
false,
CrateOrigin::Local { repo: None, name: None },
default_target_data_layout
Expand Down Expand Up @@ -259,7 +259,7 @@ impl ChangeFixture {
None,
Default::default(),
Default::default(),
Env::default(),
Env::new_for_test_fixture(),
false,
CrateOrigin::Lang(LangCrateOrigin::Core),
target_layout.clone(),
Expand Down Expand Up @@ -298,7 +298,7 @@ impl ChangeFixture {
None,
Default::default(),
Default::default(),
Env::default(),
Env::new_for_test_fixture(),
true,
CrateOrigin::Local { repo: None, name: None },
target_layout,
Expand Down
21 changes: 21 additions & 0 deletions crates/base-db/src/input.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,12 @@ pub enum CrateOrigin {
Lang(LangCrateOrigin),
}

impl CrateOrigin {
pub fn is_local(&self) -> bool {
matches!(self, CrateOrigin::Local { .. })
}
}

#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum LangCrateOrigin {
Alloc,
Expand Down Expand Up @@ -333,6 +339,17 @@ pub struct Env {
entries: FxHashMap<String, String>,
}

impl Env {
pub fn new_for_test_fixture() -> Self {
Env {
entries: FxHashMap::from_iter([(
String::from("__ra_is_test_fixture"),
String::from("__ra_is_test_fixture"),
)]),
}
}
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub struct Dependency {
pub crate_id: CrateId,
Expand Down Expand Up @@ -456,6 +473,10 @@ impl CrateGraph {
self.arena.iter().map(|(idx, _)| idx)
}

pub fn iter_mut(&mut self) -> impl Iterator<Item = (CrateId, &mut CrateData)> + '_ {
self.arena.iter_mut()
}

/// Returns an iterator over all transitive dependencies of the given crate,
/// including the crate itself.
pub fn transitive_deps(&self, of: CrateId) -> impl Iterator<Item = CrateId> {
Expand Down
20 changes: 9 additions & 11 deletions crates/hir-def/src/body.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ impl Body {
let _p = profile::span("body_with_source_map_query");
let mut params = None;

let (file_id, module, body, is_async_fn) = {
let (file_id, body, is_async_fn) = {
match def {
DefWithBodyId::FunctionId(f) => {
let data = db.function_data(f);
Expand All @@ -138,31 +138,29 @@ impl Body {
}),
)
});
(
src.file_id,
f.module(db),
src.value.body().map(ast::Expr::from),
data.has_async_kw(),
)
(src.file_id, src.value.body().map(ast::Expr::from), data.has_async_kw())
}
DefWithBodyId::ConstId(c) => {
let c = c.lookup(db);
let src = c.source(db);
(src.file_id, c.module(db), src.value.body(), false)
(src.file_id, src.value.body(), false)
}
DefWithBodyId::StaticId(s) => {
let s = s.lookup(db);
let src = s.source(db);
(src.file_id, s.module(db), src.value.body(), false)
(src.file_id, src.value.body(), false)
}
DefWithBodyId::VariantId(v) => {
let e = v.parent.lookup(db);
let src = v.parent.child_source(db);
let variant = &src.value[v.local_id];
(src.file_id, e.container, variant.expr(), false)
(src.file_id, variant.expr(), false)
}
DefWithBodyId::InTypeConstId(c) => {
(c.lookup(db).0.file_id, c.source(db).expr(), false)
}
}
};
let module = def.module(db);
let expander = Expander::new(db, file_id, module);
let (mut body, source_map) =
Body::new(db, def, expander, params, body, module.krate, is_async_fn);
Expand Down
1 change: 1 addition & 0 deletions crates/hir-def/src/body/pretty.rs
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ pub(super) fn print_body_hir(db: &dyn DefDatabase, body: &Body, owner: DefWithBo
};
format!("const {name} = ")
}
DefWithBodyId::InTypeConstId(_) => format!("In type const = "),
DefWithBodyId::VariantId(it) => {
let src = it.parent.child_source(db);
let variant = &src.value[it.local_id];
Expand Down
20 changes: 13 additions & 7 deletions crates/hir-def/src/db.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
//! Defines database & queries for name resolution.
use base_db::{salsa, CrateId, SourceDatabase, Upcast};
use either::Either;
use hir_expand::{db::ExpandDatabase, HirFileId};
use hir_expand::{db::ExpandDatabase, AstId, HirFileId};
use intern::Interned;
use la_arena::ArenaMap;
use syntax::{ast, AstPtr};
Expand All @@ -22,11 +22,12 @@ use crate::{
lang_item::{LangItem, LangItemTarget, LangItems},
nameres::{diagnostics::DefDiagnostic, DefMap},
visibility::{self, Visibility},
AnonymousConstId, AttrDefId, BlockId, BlockLoc, ConstId, ConstLoc, DefWithBodyId, EnumId,
EnumLoc, ExternBlockId, ExternBlockLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc,
LocalEnumVariantId, LocalFieldId, Macro2Id, Macro2Loc, MacroRulesId, MacroRulesLoc,
ProcMacroId, ProcMacroLoc, StaticId, StaticLoc, StructId, StructLoc, TraitAliasId,
TraitAliasLoc, TraitId, TraitLoc, TypeAliasId, TypeAliasLoc, UnionId, UnionLoc, VariantId,
AttrDefId, BlockId, BlockLoc, ConstBlockId, ConstId, ConstLoc, DefWithBodyId, EnumId, EnumLoc,
ExternBlockId, ExternBlockLoc, FunctionId, FunctionLoc, GenericDefId, ImplId, ImplLoc,
InTypeConstId, LocalEnumVariantId, LocalFieldId, Macro2Id, Macro2Loc, MacroRulesId,
MacroRulesLoc, OpaqueInternableThing, ProcMacroId, ProcMacroLoc, StaticId, StaticLoc, StructId,
StructLoc, TraitAliasId, TraitAliasLoc, TraitId, TraitLoc, TypeAliasId, TypeAliasLoc,
TypeOwnerId, UnionId, UnionLoc, VariantId,
};

#[salsa::query_group(InternDatabaseStorage)]
Expand Down Expand Up @@ -62,7 +63,12 @@ pub trait InternDatabase: SourceDatabase {
#[salsa::interned]
fn intern_macro_rules(&self, loc: MacroRulesLoc) -> MacroRulesId;
#[salsa::interned]
fn intern_anonymous_const(&self, id: (DefWithBodyId, ExprId)) -> AnonymousConstId;
fn intern_anonymous_const(&self, id: (DefWithBodyId, ExprId)) -> ConstBlockId;
#[salsa::interned]
fn intern_in_type_const(
&self,
id: (AstId<ast::ConstArg>, TypeOwnerId, Box<dyn OpaqueInternableThing>),
) -> InTypeConstId;
}

#[salsa::query_group(DefDatabaseStorage)]
Expand Down
2 changes: 1 addition & 1 deletion crates/hir-def/src/expander.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ impl Expander {
}

pub(crate) fn parse_path(&mut self, db: &dyn DefDatabase, path: ast::Path) -> Option<Path> {
let ctx = LowerCtx::with_hygiene(db, &self.cfg_expander.hygiene);
let ctx = LowerCtx::new(db, &self.cfg_expander.hygiene, self.current_file_id);
Path::from_src(path, &ctx)
}

Expand Down
4 changes: 2 additions & 2 deletions crates/hir-def/src/hir.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use crate::{
builtin_type::{BuiltinFloat, BuiltinInt, BuiltinUint},
path::{GenericArgs, Path},
type_ref::{Mutability, Rawness, TypeRef},
AnonymousConstId, BlockId,
BlockId, ConstBlockId,
};

pub use syntax::ast::{ArithOp, BinaryOp, CmpOp, LogicOp, Ordering, RangeOp, UnaryOp};
Expand Down Expand Up @@ -181,7 +181,7 @@ pub enum Expr {
statements: Box<[Statement]>,
tail: Option<ExprId>,
},
Const(AnonymousConstId),
Const(ConstBlockId),
Unsafe {
id: Option<BlockId>,
statements: Box<[Statement]>,
Expand Down
115 changes: 62 additions & 53 deletions crates/hir-def/src/hir/type_ref.rs
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ pub enum TypeRef {
Reference(Box<TypeRef>, Option<LifetimeRef>, Mutability),
// FIXME: for full const generics, the latter element (length) here is going to have to be an
// expression that is further lowered later in hir_ty.
Array(Box<TypeRef>, ConstRefOrPath),
Array(Box<TypeRef>, ConstRef),
Slice(Box<TypeRef>),
/// A fn pointer. Last element of the vector is the return type.
Fn(Vec<(Option<Name>, TypeRef)>, bool /*varargs*/, bool /*is_unsafe*/),
Expand Down Expand Up @@ -186,11 +186,7 @@ impl TypeRef {
TypeRef::RawPtr(Box::new(inner_ty), mutability)
}
ast::Type::ArrayType(inner) => {
// FIXME: This is a hack. We should probably reuse the machinery of
// `hir_def::body::lower` to lower this into an `Expr` and then evaluate it at the
// `hir_ty` level, which would allow knowing the type of:
// let v: [u8; 2 + 2] = [0u8; 4];
let len = ConstRefOrPath::from_expr_opt(inner.expr());
let len = ConstRef::from_const_arg(ctx, inner.const_arg());
TypeRef::Array(Box::new(TypeRef::from_ast_opt(ctx, inner.ty())), len)
}
ast::Type::SliceType(inner) => {
Expand Down Expand Up @@ -380,73 +376,84 @@ impl TypeBound {
}

#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ConstRefOrPath {
Scalar(ConstRef),
pub enum ConstRef {
Scalar(LiteralConstRef),
Path(Name),
Complex(AstId<ast::ConstArg>),
}

impl ConstRefOrPath {
pub(crate) fn from_expr_opt(expr: Option<ast::Expr>) -> Self {
match expr {
Some(x) => Self::from_expr(x),
None => Self::Scalar(ConstRef::Unknown),
impl ConstRef {
pub(crate) fn from_const_arg(lower_ctx: &LowerCtx<'_>, arg: Option<ast::ConstArg>) -> Self {
if let Some(arg) = arg {
let ast_id = lower_ctx.ast_id(&arg);
if let Some(expr) = arg.expr() {
return Self::from_expr(expr, ast_id);
}
}
Self::Scalar(LiteralConstRef::Unknown)
}

pub fn display<'a>(&'a self, db: &'a dyn ExpandDatabase) -> impl fmt::Display + 'a {
struct Display<'a>(&'a dyn ExpandDatabase, &'a ConstRefOrPath);
struct Display<'a>(&'a dyn ExpandDatabase, &'a ConstRef);
impl fmt::Display for Display<'_> {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self.1 {
ConstRefOrPath::Scalar(s) => s.fmt(f),
ConstRefOrPath::Path(n) => n.display(self.0).fmt(f),
ConstRef::Scalar(s) => s.fmt(f),
ConstRef::Path(n) => n.display(self.0).fmt(f),
ConstRef::Complex(_) => f.write_str("{const}"),
}
}
}
Display(db, self)
}

// FIXME: as per the comments on `TypeRef::Array`, this evaluation should not happen at this
// parse stage.
fn from_expr(expr: ast::Expr) -> Self {
// We special case literals and single identifiers, to speed up things.
fn from_expr(expr: ast::Expr, ast_id: Option<AstId<ast::ConstArg>>) -> Self {
fn is_path_ident(p: &ast::PathExpr) -> bool {
let Some(path) = p.path() else {
return false;
};
if path.coloncolon_token().is_some() {
return false;
}
if let Some(s) = path.segment() {
if s.coloncolon_token().is_some() || s.generic_arg_list().is_some() {
return false;
}
}
true
}
match expr {
ast::Expr::PathExpr(p) => {
ast::Expr::PathExpr(p) if is_path_ident(&p) => {
match p.path().and_then(|x| x.segment()).and_then(|x| x.name_ref()) {
Some(x) => Self::Path(x.as_name()),
None => Self::Scalar(ConstRef::Unknown),
None => Self::Scalar(LiteralConstRef::Unknown),
}
}
ast::Expr::PrefixExpr(prefix_expr) => match prefix_expr.op_kind() {
Some(ast::UnaryOp::Neg) => {
let unsigned = Self::from_expr_opt(prefix_expr.expr());
// Add sign
match unsigned {
Self::Scalar(ConstRef::UInt(num)) => {
Self::Scalar(ConstRef::Int(-(num as i128)))
}
other => other,
}
}
_ => Self::from_expr_opt(prefix_expr.expr()),
},
ast::Expr::Literal(literal) => Self::Scalar(match literal.kind() {
ast::LiteralKind::IntNumber(num) => {
num.value().map(ConstRef::UInt).unwrap_or(ConstRef::Unknown)
num.value().map(LiteralConstRef::UInt).unwrap_or(LiteralConstRef::Unknown)
}
ast::LiteralKind::Char(c) => {
c.value().map(ConstRef::Char).unwrap_or(ConstRef::Unknown)
c.value().map(LiteralConstRef::Char).unwrap_or(LiteralConstRef::Unknown)
}
ast::LiteralKind::Bool(f) => ConstRef::Bool(f),
_ => ConstRef::Unknown,
ast::LiteralKind::Bool(f) => LiteralConstRef::Bool(f),
_ => LiteralConstRef::Unknown,
}),
_ => Self::Scalar(ConstRef::Unknown),
_ => {
if let Some(ast_id) = ast_id {
Self::Complex(ast_id)
} else {
Self::Scalar(LiteralConstRef::Unknown)
}
}
}
}
}

/// A concrete constant value
/// A literal constant value
#[derive(Debug, Clone, PartialEq, Eq, Hash)]
pub enum ConstRef {
pub enum LiteralConstRef {
Int(i128),
UInt(u128),
Bool(bool),
Expand All @@ -460,18 +467,20 @@ pub enum ConstRef {
Unknown,
}

impl ConstRef {
impl LiteralConstRef {
pub fn builtin_type(&self) -> BuiltinType {
match self {
ConstRef::UInt(_) | ConstRef::Unknown => BuiltinType::Uint(BuiltinUint::U128),
ConstRef::Int(_) => BuiltinType::Int(BuiltinInt::I128),
ConstRef::Char(_) => BuiltinType::Char,
ConstRef::Bool(_) => BuiltinType::Bool,
LiteralConstRef::UInt(_) | LiteralConstRef::Unknown => {
BuiltinType::Uint(BuiltinUint::U128)
}
LiteralConstRef::Int(_) => BuiltinType::Int(BuiltinInt::I128),
LiteralConstRef::Char(_) => BuiltinType::Char,
LiteralConstRef::Bool(_) => BuiltinType::Bool,
}
}
}

impl From<Literal> for ConstRef {
impl From<Literal> for LiteralConstRef {
fn from(literal: Literal) -> Self {
match literal {
Literal::Char(c) => Self::Char(c),
Expand All @@ -483,14 +492,14 @@ impl From<Literal> for ConstRef {
}
}

impl std::fmt::Display for ConstRef {
impl std::fmt::Display for LiteralConstRef {
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> {
match self {
ConstRef::Int(num) => num.fmt(f),
ConstRef::UInt(num) => num.fmt(f),
ConstRef::Bool(flag) => flag.fmt(f),
ConstRef::Char(c) => write!(f, "'{c}'"),
ConstRef::Unknown => f.write_char('_'),
LiteralConstRef::Int(num) => num.fmt(f),
LiteralConstRef::UInt(num) => num.fmt(f),
LiteralConstRef::Bool(flag) => flag.fmt(f),
LiteralConstRef::Char(c) => write!(f, "'{c}'"),
LiteralConstRef::Unknown => f.write_char('_'),
}
}
}

0 comments on commit dcd3155

Please sign in to comment.