search

HaxeFoundation / haxe

Haxe - The Cross-Platform Toolkit
https://haxe.org
6.03k stars 648 forks source link

Do not store warnings as cache bound objects during display requests #11687

Closed kLabz closed 3 weeks ago

kLabz commented 3 weeks ago

See #11675 (not closing because I'd like to add a test first)

Simn commented 3 weeks ago

The obvious question here is if this is really the fix we want or if the warning shouldn't occur in the first place.

kLabz commented 3 weeks ago

We're not reading expressions for many modules during display requests; this kind of warnings are bound to happen.

The correct way to handle it would indeed be to do more advanced checks where we emit those warnings (and it's not only about WInlinedOptimizedField), but it's a lot more work and likely a lot of trial and error.

I don't think this PR is wrong either way: display requests are not doing work that is persisted so they should not write cache bound objects.

Simn commented 3 weeks ago

I don't think this PR is wrong either way: display requests are not doing work that is persisted so they should not write cache bound objects.

That seems quite agreeable, but here we're only fixing it for warnings in particular. I can't really think of any problems that would come from resources or include files, but we could run into this again in the future if we add more CBOs.

Then again the nature of warnings makes this rather particular indeed, and perhaps it's really the only potential issue that could arise from display requests. I'm fine with merging this but let's keep this in mind.

kLabz commented 3 weeks ago

Yeah I plan to look into other cache bound objects while adding tests, but not sure I'll find problems with those.

kLabz commented 3 weeks ago

Merging this for now, but keeping #11675 open until my next PR that will handle tests (and potential issues with other cache bound objects).