Fix foreign interface dependencies which are disabled by feature-gating #2394
Conversation
danielvallance
requested review from
pchickey
and removed request for
a team
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Thanks!
I'm a bit worried about this approaching being brittle though in how it relies on iterating over all worlds and checking for stability. Would it be possible to push None, instead of Some(iface_id), and then handle the None case later when it shows up?
|
Hi @alexcrichton , yeah my initial approach was to just push None when the interface was not found in pkg.interfaces, however it caused some unit tests to fail as it results in successfully parsing some packages which import nonexistent interfaces. That's what led me to create the is_gated closure as a form of basic validation that the interface actually exists in some form. I could not see another place where it would be more appropriate to check the interface actually exists however I may be overlooking something as I am not super familiar with the code. Just to confirm, we definitely want to fail in the case where a nonexistent interface is imported but not used, right? (the unit tests which failed my initial approach were "tests/ui/parse-fail/bad-pkg3", "tests/ui/parse-fail/bad-pkg2" and "tests/ui/parse-fail/use-world") |
|
Oof ok yeah that's pretty tricky. Those tests should indeed continue to keep failing and I see how "just push Would it be possible to update how |
|
hi, yeah i can do some research in that area, thanks for the pointer! if i can find a more "principled" way of fixing this then i will update this PR |
danielvallance
force-pushed
the
wit_bug
branch
from
a14c687 to
32b991c
Compare
|
i think i am in the right area with this latest change, however i am going to try and add some bookkeepping to keep track of the stability of different imports of the same dependency. i will mark as draft while i do this |
danielvallance
force-pushed
the
wit_bug
branch
from
32b991c to
4570b09
Compare
|
Latest change records the stability of each import of the foreign dependency, and a placeholder is pushed if and only if imports exist, and they are all feature gated |
alexcrichton
left a comment
alexcrichton
left a comment
There was a problem hiding this comment.
Looks like a reasonable solution to me, thanks again for this!
danielvallance
force-pushed
the
wit_bug
branch
from
4570b09 to
4f9f171
Compare
| if u.arbitrary()? { | ||
| return Ok(None); | ||
| } | ||
|
|
There was a problem hiding this comment.
er this comment didn't go through...
anyway, is this a stray change? Or intentional? (naively this looks related to helping to test this PR but not something that should be included, but wanted to confirm)
There was a problem hiding this comment.
hi there, this is intentional!
after i initially addressed your comments in commit (4f9f171), it caused a test failure which seemed to me to be the added call to include_stability picking up on an invalid fuzzed WIT file which had a stable since annotation, but no overall version
This extra commit stops that from happening (i mentioned the rationale in this reply #2394 (comment) but maybe i should have put it in a separate thread)
There was a problem hiding this comment.
Oh oops sorry I missed that. Agreed that error looks like invalid WIT is generated, but unconditionally generating stability annotations means that non-annotated things will no longer get fuzzed. Would you be ok updating this to always return Ok(None) and file an issue with a FIXME for the invalid-generated WIT?
There was a problem hiding this comment.
hi there, had a closer look at this and i dont think it was invalid wit being generated, i was just passing the wrong pkgid into include_stability which meant it was looking at the wrong package when checking the consistency between the package's version and any version gating it did
i have changed this now, and also i had to change one of the test output files as invoking include_stability earlier on results in a cosmetic difference between the test outputs
danielvallance
force-pushed
the
wit_bug
branch
from
9578fc3 to
bd7634f
Compare
Previously, the process_foreign_interfaces function would
receive unresolved interfaces with their stability values
set to Unknown
This made it hard to distinguish between interfaces which
had been feature gated out, and genuinely nonexistent
interfaces
This commit records the stability of each import of a foreign dependency
when populating the self.foreign_{deps, interfaces, worlds}
maps.
This enables process_foreign_interfaces to push None as a placeholder
if it sees that all of its imports have been feature-gated out. Otherwise,
it will allow the resolver to try and resolve the import, and if the
interface genuinely does not exist, it will fail.
Fixes issue 2285
danielvallance
force-pushed
the
wit_bug
branch
from
bd7634f to
cd0638f
Compare
2 participants
A foreign imported interface can be missing from pkg.interfaces either because it does not exist, or because it was disabled by feature-gating.
Previously, the parser failed in both cases, when it should only fail when it does not exist, and should succeed when the references to it are disabled by feature-gating.
By checking the set of unresolved worlds' imports and exports for references to this interface, it is possible to determine if they contain any references to the interface, and whether or not they were disabled by feature gating.
If they are all disabled, then it does not matter that the interface is absent from pkg.interfaces, and the code should not fail. If any references are active however, then this is an error as the code will not be able to find the interface in pkg.interfaces.
If no references exist at all, then no combination of feature enabling/disabling would satisfy the import/use statement which requires the interface, and the code should fail.
This fixes issue 2285 and a unit test was added to cover this case.