-
Notifications
You must be signed in to change notification settings - Fork 179
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
Add default RustIrDatabase::*_name
impls
#575
Add default RustIrDatabase::*_name
impls
#575
Conversation
This is a prelude to adding default `ChalkIrDatabase::*_name` methods which call the respective Interner debug methods. The other top-level item debug methods existed, but there were no debug methods taking `FnDefId`, so we've added one. This also makes two small, related changes to the OpaqueTyId and AssocTypeId debug methods - mainly, changing their error message from InvalidItemId to InvalidXxxId. This matches the new FnDefId error message, and the other ones in earlier methods. Co-authored-by: David Ross <daboross@daboross.net>
The .chalk syntax writer needs names for items such as adts, opaque types, and traits. `RustIrDatabase::name_*` methods were added to provide these names, but they increase the burden on implementors. To remove this burden, we added default impelemtnations to `RustIrDatabase` that sanitize the output of the `Interner::debug_*` methods so they are only alphanumeric ascii characters. We also removed the custom implementations for the .chalk syntax writer tests to ensure that the default implementations are tested. We had to keep the implementation for associated types because the debug methods outputted different names. This broke the reparse test because the `Program`s were no longer considered equal by the derived `Eq` implementation. Co-authored-by: David Ross <daboross@daboross.net>
Forgot to run all the tests! There's an additional bug related to the WriterState we need to fix in order to get this working. |
The previous commit switched from custom `RustIrDatabase::name_*` implementations that directly used the data in `Program` to using the debug methods in the `Interner`. This exposed a bug in the test harness for logging db. The debug methods rely on thread local storage to get access to the data inside of `Program`. The bug caused the program to run outside of the thread local storage context that provided program, causing the names to change to a fallback value. This broke the re-solve tests because the names needed to match up with the goal. This commit fixes the tests and underlying bug by correctly running `LoggingRustIrDatabase::to_string` in the tls context. Co-authored-by: David Ross <daboross@daboross.net>
2d40ae0
to
12e932f
Compare
Fixed that bug! This should be ready. Incidentally, that bug exposed another bug related to how we're doing name deduplication. We'll submit a fix for that in a separate PR as soon as we've written tests. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! This should also help on RA's side, because displaying the names there is tricky as well.
// The default implementation for `RustIrDatabase::assoc_type_name` outputs | ||
// the name in the format `(Trait::AssocTypeName)`, which is reformatted to | ||
// `_Trait__AssocTypeName_`. This doesn't match the input names, which is | ||
// normally acceptable, but causes the re-parse tests for the .chalk syntax | ||
// writer to fail. This is because they use the `Eq` implementation on | ||
// Program, which checks for name equality. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the Trait::AssocTypeName
output is (mostly) valid, maybe the formatting can be skipped for assoc_type_name
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The display test suite currently relies on exact name equality, so we need to do something to get the assoc_type_name
type name output to exactly match the input.
One alternative would be to make a RustIrDatabase
wrapper for Program
which just overwrites assoc_type_name
, but I'm not sure that would be worth it. Especially since having this one method overwrite also means that the debug printer can have fully exact names when printing output from the chalk command line, and this code is run in no other situations. At least to my knowledge, neither rustc nor rust-analyzer integration uses Program
, right?
We could try to create a test equality that doesn't depend on names, but it would also be more effort - and I'm not sure we want to allow names to slip in those tests without us noticing. They're otherwise fairly exact output correctness tests.
It shouldn't be that tricky, we just haven't implemented it 😉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, modulo @detrumi's suggestions.
Apply fixes for code-quality nitpicks raised by detrumi in the pull request: rust-lang#575
Apply fixes for code-quality nitpicks raised by detrumi in the pull request: rust-lang#575 Co-authored-by: David Ross <daboross@daboross.net>
fe230a9
to
e5d7091
Compare
@detrumi we have bors now :) |
.chalk syntax writer: Fix name de-duplication bug While working on #575, we found a bug in the name de-duplication logic. To ensure we have unique names, we store a mapping between ids and names for ids that don't have unique names. This state was managed inside of `write_items`, but we call `write_items` twice when we output the data from the `LoggingRustIrDatabase`. It is called once to output the logged items, and once to output the necessary stub items. The id to name mappings are lost between these calls, which breaks the unique name guarantee. We fixed this by creating the mapping information store outside of the display code, and passing it into calls to `write_items`. We also significantly improved the name de-duplication tests.
This adds default implementations for
RustIrDatabase::*_name
methods. They useInterner::debug_*
methods, and sanitize the output by replacing all non-alphanumeric-ascii chars with_
.This also removes all custom implementations of these methods, except the one for
Program::assoc_type_name
. We've kept that one, as the default impl ends up returning_TraitName__AssocName_
, and for display tests, we need it to be exactlyAssocName
.We've discussed the justification for this in the .chalk file writer zulip discussion, and the first .chalk file writer pull request. But for anyone reading this, it boils down to the fact that we don't want an to impose an undue burden on
RustIrDatabase
implementors, especially for a debug tool. Rustc integration shouldn't have to accommodateLoggingRustIrDatabase
.