Skip to content

Use salsa tracking to link InputFile to InputIngot#1070

Closed
micahscopes wants to merge 3 commits intoargotorg:masterfrom
micahscopes:salsa-track-inputs
Closed

Use salsa tracking to link InputFile to InputIngot#1070
micahscopes wants to merge 3 commits intoargotorg:masterfrom
micahscopes:salsa-track-inputs

Conversation

@micahscopes
Copy link
Copy Markdown
Collaborator

No description provided.

pub struct InputFile {
/// A path to the file from the ingot root directory.
#[return_ref]
#[set(__set_path_impl)]
Copy link
Copy Markdown
Collaborator Author

@micahscopes micahscopes Mar 31, 2025

Choose a reason for hiding this comment

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

If InputFile instances are loosely tied to InputIngot instances, we need to discourage people from modifying InputFile.path directly.

This could be handled by some file_rename method on InputIngot. It could also be real nice to replace InputIngot::files with a map or patricia trie to get nice cheap getters. If we're already going funnel InputFile::path modification through an InputIngot::file_rename method, maintaining that index would be better justified.

Comment on lines +119 to +124
// Check if the ingot already has a file with the same path
for file in ingot.files(db).iter() {
if file.path(db).as_path() == path.path(db) {
return *file;
}
}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

This was necessary so that ingots with manually created/added InputFiles (right now just the core ingot) will have their preset InputFiles returned.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

#[folder = "../../library/core"]
struct Core;

#[salsa::tracked]
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Disabling salsa tracking on the core ingot gets rid of the errors in CI. I can't see why these particular tests should fail as a result of reusing the core ingot:

running 17 tests
test run_name_resolution__import_alias_cycle ... ok
test run_name_resolution__conflict_variant ... ok
test run_name_resolution__import_cycle ... ok
test run_name_resolution__conflict_field ... ok
test run_name_resolution__import_missing ... ok
test run_name_resolution__import_invisible ... ok
test run_name_resolution__import_conflict ... ok
test run_name_resolution__conflict_generics ... ok
test run_name_resolution__path_shadow ... ok
test run_name_resolution__conflict ... ok
test run_name_resolution__import_unimpotable ... ok
test run_name_resolution__record_field_visibility ... ok
test run_name_resolution__path_invalid_domain ... ok
test run_name_resolution__path_missing_generics ... ok
test run_name_resolution__trait_visibility ... ok
test run_name_resolution__import_ambiguous ... FAILED
test run_name_resolution__import_ambiguous_builtin ... FAILED

failures:

---- run_name_resolution__import_ambiguous stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: fixtures/name_resolution/import_ambiguous.snap
Snapshot: import_ambiguous
Source: /home/micah/hacker-stuff-2023/fe-stuff/fe/crates/uitest:21
Input file: fixtures/name_resolution/import_ambiguous.fe
──────────────────────────────────────────────────────────────────────────
Expression: diags
──────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────
    0       │-error[2-0004]: `S` is ambiguous
    1       │-   ┌─ import_ambiguous.fe:2:9
    2       │-   │
    3       │- 2 │ pub use S
    4       │-   │         ^ `S` is ambiguous
    5       │-   ·
    6       │-11 │         pub struct S {}
    7       │-   │                    - candidate 1
    8       │-   ·
    9       │-14 │         pub struct S {}
   10       │-   │                    - candidate 2
   11       │-
   12       │-error[2-0004]: `S` is ambiguous
   13       │-   ┌─ import_ambiguous.fe:7:13
   14       │-   │
   15       │- 7 │     pub use S
   16       │-   │             ^ `S` is ambiguous
   17       │-   ·
   18       │-11 │         pub struct S {}
   19       │-   │                    - candidate 1
   20       │-   ·
   21       │-14 │         pub struct S {}
   22       │-   │                    - candidate 2
────────────┴─────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'run_name_resolution__import_ambiguous' panicked at /home/micah/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.42.2/src/runtime.rs:679:13:
snapshot assertion for 'import_ambiguous' failed in line 21
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

---- run_name_resolution__import_ambiguous_builtin stdout ----
━━━━━━━━━━━━━━━━━━━━━━━━━━━━ Snapshot Summary ━━━━━━━━━━━━━━━━━━━━━━━━━━━━
Snapshot file: fixtures/name_resolution/import_ambiguous_builtin.snap
Snapshot: import_ambiguous_builtin
Source: /home/micah/hacker-stuff-2023/fe-stuff/fe/crates/uitest:21
Input file: fixtures/name_resolution/import_ambiguous_builtin.fe
──────────────────────────────────────────────────────────────────────────
Expression: diags
──────────────────────────────────────────────────────────────────────────
-old snapshot
+new results
────────────┬─────────────────────────────────────────────────────────────
    0       │-error[2-0004]: `i32` is ambiguous
          0 │+error[2-0005]: `i32` can't be used as a middle segment of a path
    1     1 │   ┌─ import_ambiguous_builtin.fe:3:5
    2     2 │   │
    3     3 │ 3 │ use i32::*
    4       │-  │     ^^^ `i32` is ambiguous
          4 │+  │     ^^^ `i32` can't be used as a middle segment of a path
────────────┴─────────────────────────────────────────────────────────────
To update snapshots run `cargo insta review`
Stopped on the first failure. Run `cargo insta test` to run all snapshots.
thread 'run_name_resolution__import_ambiguous_builtin' panicked at /home/micah/.cargo/registry/src/index.crates.io-6f17d22bba15001f/insta-1.42.2/src/runtime.rs:679:13:
snapshot assertion for 'import_ambiguous_builtin' failed in line 21


failures:
    run_name_resolution__import_ambiguous
    run_name_resolution__import_ambiguous_builtin

test result: FAILED. 15 passed; 2 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.06s

c.c. @sbillig @g-r-a-n-t this is pretty weird

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant