Update tests for upcoming #5059 and #5060 on member access and finding impl witnesses in facets #5054
Conversation
There was a problem hiding this comment.
Can you update the PR description to reflect what tests you're changing, or what the upcoming changes are to? Maybe I'm thinking about this more because of "2 small simplifications" on #5055, but I think the PR title should have a little more information.
| // --- core.carbon | ||
| package Core; | ||
|
|
||
| interface ImplicitAs(Dest:! type) { | ||
| fn Convert[self: Self]() -> Dest; | ||
| } |
There was a problem hiding this comment.
I believe the default preference is to import the prelude, because this test isn't intended to test non-standard ImplicitAs definitions.
There was a problem hiding this comment.
(I see several tests like this, so just noting it here)
There was a problem hiding this comment.
My most recent convo about this (i think it was open discussion a few weeks ago) was to prefer no_prelude when possible, and just bring in a bit of Core as needed to get the correct error messages. Without the ImplicitAs, the error messages change from "cannot implicitly convert X to Y" to "could not find Core.ImplicitAs".
There was a problem hiding this comment.
no_prelude tests are substantially easier to debug. Adding a Core package with ImplicitAs makes the error more informative (since you can see the query), but doesn't involve anything about that interface.
There was a problem hiding this comment.
Maybe we should have a meeting about this? @danakj I think the advice of "prefer no_prelude when possible" is just half: the intent is to avoid maintaining copies of the prelude too, so the second half is "but use the prelude instead of duplicating it".
Note @josh11b either way, if you keep the Core, I think it should probably be at the top of every file. I saw some where it was mixed in the middle.
There was a problem hiding this comment.
No problem to move Core to the top. This particular bit of Core is quite short and the benefit for keeping just this much is pretty high.
There was a problem hiding this comment.
@danakj Glancing at the meeting minutes, I see what you're saying at 2025-01-23. Unfortunately the discussion is uncredited so it's hard to get context.
My recollection of past discussions with @zygoloid is closer to what I've said (and neither of us were present, according to the minutes, which may account for the difference in response). For example, with operator tests we don't want to copy out the operator interfaces in order to get a subset of operator interface functionality. But yes, we can discuss in the toolchain meeting.
There was a problem hiding this comment.
@josh11b For context, can you elaborate on what debugging issues you're running into with making this a prelude tests? The cross-file dep is going to be there.
To share a concern, the ImplicitAs interface here is incomplete (the official version has a TODO to add As), so I think this copy -- while small and possibly low maintenance, will probably also end up being brittle.
My current path of thought is that, if your goal is to only get part of the prelude, maybe we could make it possible to explicitly import something like "prelude/operators/as.carbon" (i.e., to get the interface without any implementations).
There was a problem hiding this comment.
Being able to explicitly import a small part of the prelude would address my use case.
When debugging, I often want to set a breakpoint on some construct of interest, like impl lookup. It is much quicker if my case is the first or second rather than after the large number generated by the prelude.
There was a problem hiding this comment.
Would it make sense to review (and if good, land) this PR while we revisit the discussion around the prelude?
It seems somewhat easy to do a follow-up PR to switch these back to importing the prelude if that's appropriate, and landing these as is seems like it would still be an improvement and unblock further work?
I've created the PRs with the future changes and added them to the PR description. |
PR title appears unchanged. To be clear, when we look at a history such as https://github.com/carbon-language/carbon-lang/pulls?q=is%3Apr+is%3Aclosed or https://github.com/carbon-language/carbon-lang/commits/trunk/, I think it'd be helpful to have some information on what changed -- right now, I need to open the PR to figure out what portion of the codebase is affected. |
Here are some test changes so the diffs that come from my upcoming functionality changes are easier to see. Upcoming functionality includes:
I'm also making the toolchain/check/testdata/impl/compound.carbon test into a no_prelude version, and adding a version that tests with importing.