.chalk syntax writer: Fix name de-duplication bug #585
Conversation
In preparation for future changes, we wanted to split the WriterState into a public and a private version. This is needed to support persisting WriterState between writes. We also encapsulated the `db` field behind a method, so it can be moved later on. Co-authored-by: David Ross <daboross@daboross.net>
This fixes two bugs in unique name generation. The bug which prompted this change is related to stub items: LoggingRustIrDatabase calls `write_stub_items` and `write_items` separately, and unique name choices were not persisted between these calls. The second bug, discovered by our test framework, was that associated type names were not made unique within a trait. We refactored writer state into a public `WriterState`, and a private `InternalWriterState`. The public WriterState holds a copy of the DB and the name de-duplication information. The private `InternalWriterState` holds the current debrujin index information and formatting data. The public `WriterState` is created outside of the display code, and is saved between writes. In the case of the LoggingDB, it is stored inside the logging db. This ensures that all writes from the LoggingDB have consistent unique names. Storing `WriterState` persistently is not strictly necessary; to fix the unique name choice bug, we only need to persist it between the calls to `write_stub_items` and `write_items` in `Display::fmt`. But storing it longer was easy to do, and not less correct. Co-authored-by: David Ross <daboross@daboross.net>
| } | ||
|
|
||
| fn hidden_opaque_type(&self, id: OpaqueTyId<I>) -> Ty<I> { | ||
| self.db.borrow().hidden_opaque_type(id) | ||
| self.db.hidden_opaque_type(id) | ||
| } | ||
|
|
||
| fn impls_for_trait( |
There was a problem hiding this comment.
You missed a db.borrow() call here, and in closure_inputs_and_output as well
chalk-solve/src/display/state.rs
Outdated
| /// given database, and must keep the same ID<->item relationships. | ||
| pub(super) fn wrap_db_ref<'a, DB2: ?Sized, P2, F>(&'a self, f: F) -> WriterState<I, DB2, P2> | ||
| where | ||
| P: 'a, |
There was a problem hiding this comment.
This bound doesn't seem to be needed
There was a problem hiding this comment.
I think we need it because f takes &'a P, that being sound requires P: 'a, and since we're just introducing 'a here, there's no guarantee it really is shorter than P?
We need 'a at all, because we want to be able to create a P2 which directly wraps P, and in order to do that the &P needs a lifetime that's guaranteed to outlive the function. If it was just FnOnce(&P) -> P2, then the &P could have been created inside wrap_db_ref, and then it really would be unsound to have P2 wrapping P.
Edit: I was 100% sure we'd checked that it doesn't compile without P: 'a, but it must have been some other change. Looks like it is redundant!
We fixed nitpick froms rust-lang#585: * Removed excess .borrow() calls introduced by a git merge. * Removed uneeded life-time bound. Thanks detrumi for the feedback! Co-authored-by: David Ross <daboross@daboross.net>
|
@bors r+ |
|
📌 Commit 888da92 has been approved by |
|
☀️ Test successful - checks-actions |
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 callwrite_itemstwice when we output the data from theLoggingRustIrDatabase. 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.