Remove potential partial snapshot in codefixservice #59586
Conversation
CyrusNajmabadi
force-pushed
the
partialSnapshot
branch
from
f7000ee
to
02603da
Compare
|
@jasonmalinowski @mavasani this is ready to review. |
| // Note: it feels very strange that we could ever not find a fixer in our list. However, this | ||
| // occurs in testing scenarios. I'm not sure if the tests represent a bogus potential input, or if | ||
| // this is something that can actually occur in practice and we want to keep working. | ||
| return null; |
There was a problem hiding this comment.
@mavasani for thoughts on this. Specifically, if i throw here, tests in CodeFixService tests fail. but i think they're likely constructing this service wrong.
There was a problem hiding this comment.
for example it does:
var fixService = new CodeFixService(
diagnosticService, logger, fixers, SpecializedCollections.EmptyEnumerable<Lazy<IConfigurationFixProvider, CodeChangeProviderMetadata>>());So we have a list of fixers, but no metadata about htem. i can't tell if htis is a reasonable or rational think that coudl actually happen in practice.
There was a problem hiding this comment.
Do you recall which tests were failing? From what I vaguely recall, we had some scenario where if a code fixer throws some exception during creation due to some missing types or something, we wouldn't be able to create an instance of the fixer, and had to handle this error case with nulls. I am wondering if the failures you saw were coming from tests explicitly validating exception during fixer creation.
There was a problem hiding this comment.
yeah, it was tests in CodeFixServiceTets. THere we manually instnatiated some test-only CodeFixProviders, then instantiated the CodeFixService. But we passed along an empty metadata-array, presumably because we didn't care in those tests and the code was resilient to not having it. But i can't tell if that's a real scenario taht occurs in practice.
There was a problem hiding this comment.
That lazy that this is getting rid of seemed hugely sketchy, in that it assumed that when it's realized all the fixers must have been realized; this is a lot more explicit that it's safe to skip the uncreated lazies since we couldn't have gotten the fixer any other way.
I agree with you that "return null" looks fishy for those tests, but would defer to @mavasani for what to do there.
ghost
added this to the 
Followup to #59582