Skip to content

fix: Fix a bug when synthetic AST node were searched in the AST ID map and caused panics #18555

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.

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

Merged
merged 1 commit into from
Dec 3, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 25 additions & 1 deletion crates/hir-def/src/lower.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
use std::{cell::OnceCell, mem};

use hir_expand::{span_map::SpanMap, AstId, HirFileId, InFile};
use span::{AstIdMap, AstIdNode};
use span::{AstIdMap, AstIdNode, Edition, EditionedFileId, FileId, RealSpanMap};
use stdx::thin_vec::ThinVec;
use syntax::ast;
use triomphe::Arc;
Expand Down Expand Up @@ -63,6 +63,30 @@ impl<'a> LowerCtx<'a> {
}
}

/// Prepares a `LowerCtx` for synthetic AST that needs to be lowered. This is intended for IDE things.
pub fn for_synthetic_ast(
db: &'a dyn DefDatabase,
ast_id_map: Arc<AstIdMap>,
types_map: &'a mut TypesMap,
types_source_map: &'a mut TypesSourceMap,
) -> Self {
let file_id = EditionedFileId::new(
FileId::from_raw(EditionedFileId::MAX_FILE_ID),
Edition::Edition2015,
);
LowerCtx {
db,
// Make up an invalid file id, so that if we will try to actually access it salsa will panic.
file_id: file_id.into(),
Comment on lines +79 to +80
Copy link
Member

Choose a reason for hiding this comment

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

Not a fan of this i have to say, I'd rather we pass the original file id even if it shouldn't ever be used. If we do use it and trigger a panic it will end up confusing devs that don't know of this part

Copy link
Contributor Author

@ChayimFriedman2 ChayimFriedman2 Dec 2, 2024

Choose a reason for hiding this comment

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

If we use the original file we can also trigger panics, only they will be less consistent and harder to debug. That was my motivation for doing it, although I agree it's not great.

Also note that the file ID is private to LowerCtx, so it should be easier to protect.

span_map: SpanMap::RealSpanMap(Arc::new(RealSpanMap::absolute(file_id))).into(),
ast_id_map: ast_id_map.into(),
impl_trait_bounds: Vec::new(),
outer_impl_trait: false,
types_map,
types_source_map,
}
}

pub(crate) fn span_map(&self) -> &SpanMap {
self.span_map.get_or_init(|| self.db.span_map(self.file_id))
}
Expand Down
13 changes: 10 additions & 3 deletions crates/hir/src/semantics.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,15 @@ use intern::Symbol;
use itertools::Itertools;
use rustc_hash::{FxHashMap, FxHashSet};
use smallvec::{smallvec, SmallVec};
use span::{EditionedFileId, FileId, HirFileIdRepr, SyntaxContextId};
use span::{AstIdMap, EditionedFileId, FileId, HirFileIdRepr, SyntaxContextId};
use stdx::TupleExt;
use syntax::{
algo::skip_trivia_token,
ast::{self, HasAttrs as _, HasGenericParams, IsString as _},
AstNode, AstToken, Direction, SyntaxKind, SyntaxNode, SyntaxNodePtr, SyntaxToken, TextRange,
TextSize,
};
use triomphe::Arc;

use crate::{
db::HirDatabase,
Expand Down Expand Up @@ -1973,10 +1974,16 @@ impl SemanticsScope<'_> {
/// Resolve a path as-if it was written at the given scope. This is
/// necessary a heuristic, as it doesn't take hygiene into account.
pub fn speculative_resolve(&self, ast_path: &ast::Path) -> Option<PathResolution> {
Copy link
Member

Choose a reason for hiding this comment

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

Not a blocker for this, but I wonder if this is a good API here. It feels brittle passing an ast node that isn't even from an actual file/expansion and then lowering it.

let root = ast_path.syntax().ancestors().last().unwrap();
let ast_id_map = Arc::new(AstIdMap::from_source(&root));
let (mut types_map, mut types_source_map) =
(TypesMap::default(), TypesSourceMap::default());
let mut ctx =
LowerCtx::new(self.db.upcast(), self.file_id, &mut types_map, &mut types_source_map);
let mut ctx = LowerCtx::for_synthetic_ast(
self.db.upcast(),
ast_id_map,
&mut types_map,
&mut types_source_map,
);
let path = Path::from_src(&mut ctx, ast_path.clone())?;
resolve_hir_path(
self.db,
Expand Down
45 changes: 45 additions & 0 deletions crates/ide-assists/src/handlers/add_missing_impl_members.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2318,4 +2318,49 @@ impl<'a> Test<'a, i32> for bool {
"#,
);
}

#[test]
fn issue_17321() {
check_assist(
add_missing_impl_members,
r#"
fn main() {}

mod other_file_1 {
pub const SOME_CONSTANT: usize = 8;
}

mod other_file_2 {
use crate::other_file_1::SOME_CONSTANT;

pub trait Trait {
type Iter: Iterator<Item = [u8; SOME_CONSTANT]>;
}
}

pub struct MyStruct;

impl other_file_2::Trait for MyStruct$0 {}"#,
r#"
fn main() {}

mod other_file_1 {
pub const SOME_CONSTANT: usize = 8;
}

mod other_file_2 {
use crate::other_file_1::SOME_CONSTANT;

pub trait Trait {
type Iter: Iterator<Item = [u8; SOME_CONSTANT]>;
}
}

pub struct MyStruct;

impl other_file_2::Trait for MyStruct {
$0type Iter;
}"#,
);
}
}