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.

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

analyze: add DefId filter #1014

Merged
merged 5 commits into from
Aug 23, 2023
Merged

analyze: add DefId filter #1014

merged 5 commits into from
Aug 23, 2023

Conversation

spernsteiner
Copy link
Collaborator

Adds an environment variable C2RUST_ANALYZE_FIXED_DEFS_LIST, which can be set to a file path containing a list of DefIds. Each def on this list will have its pointers marked FIXED. Rewriting is also skipped for these defs (for now - this might not be strictly necessary).

The def list file (if provided) is expected to contain DefIds in their fmt::Debug format, one per line. That is, each line should look like DefId(0:6 ~ alias1[0dc4]::alias1_good) (the short form DefId(0:6) is also accepted, but is less readable). Blank lines and comments (starting with #) are ignored. For convenience, c2rust-analyze now prints all the DefIds in the crate at startup, so that output can be used to build the def list file.

Copy link
Contributor

@kkysen kkysen left a comment

Choose a reason for hiding this comment

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

How stable are DefIds? I don't mean the printed format itself, but the two indices that comprise a DefId. Do those indices change if the source changes?

I think it'd be useful to have a few more checks done on the parsed DefIds so that we don't accidentally get invalid ones that are silently handled wrong. If the DefId in the file contains the pretty printed name of that item, can we check that the parsed DefId formats as that same string? Also, can we check that every parsed DefId corresponds to an actual DefId?

Overall, I'm not sure this is the cleanest way to skip items (I think an attribute you can put on modules, functions, any item that inner items inherit would be cleaner), I think this is good enough for now for what we want.

@spernsteiner
Copy link
Collaborator Author

How stable are DefIds? I don't mean the printed format itself, but the two indices that comprise a DefId. Do those indices change if the source changes?

Good point - they're not stable at all, so the current approach would behave unpredictably if the def list isn't up to date with the current state of the code. I'll change it to check after parsing that the path part of the DefId matches what's in the file. Having the file contain DefPaths or something would make it more robust against source changes, but this approach is much easier to implement and will at least notify the user that a mismatch occurred.

I think an attribute you can put on modules, functions, any item that inner items inherit would be cleaner

That would be easy enough to add. #[c2rust_analyze_test::fixed_signature] already has a mostly similar effect. My thinking is that for bulk edits such as enabling/disabling an entire module, it's easier to work with a big list of DefIds than to add/remove attributes on dozens of different items.

@spernsteiner spernsteiner merged commit 3052d7e into master Aug 23, 2023
9 checks passed
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.

3 participants