Set implied node format during source file creation

[frodi-karlsson]

Here I introduce setting SourceFile.ImpliedNodeFormat SourceFileMetaData.ImpliedNodeFormat as well as a field implied in commented code, PackageJsonScope. The location of ImpliedNodeFormat is as consistent as I could make it, considering some program flow is different in parseSourceFile etc.

In doing this, some things were unlocked like commented synthetic default code, and packageJson.type module detection, both of which delivered some nice diff improvements. (omitted for scope)

That being said, I'm not confident in making such a large change, but felt it was better to risk pushing it up and being told to sod off rather than saying "maybe I can fix synthetic defaults once the team implements this property" 😄

[frodi-karlsson]

Weird, can't reproduce the failure. Probably this change is not concurrency safe somehow...

[jakebailey]

I haven't looked at much of the PR, but the main thing to consider is that ASTs are reused both in tests and in the LS, so we can't store anything on the AST that could differ between different uses. The fact that you have fields set by the compiler host is the red flag; that info needs to be stored out of band since it can differ depending on context.

[frodi-karlsson]

I see, thanks a lot for the direction.

I'll leave this drafted for now if that's alright to perhaps give it another shot

[jakebailey]

Agh, clearly I should have read what you had beforehand. That's what I get for commenting from my phone early in the morning.

What you had previously was probably actually fine; having it as an argument to the GetSourceFile option was probably actually okay, it may just have been that you did not add its info to sourceFileCacheKey in testing.

Having it be a part of the file loader unfortunately makes less sense; by the time GetSourceFile returns, it's no longer safe to concurrently modify the source file in any way.

[jakebailey]

My commentary was actually to do things differently entirely and store the implied format on Program per-file, then provide access to it via a host method. That would also work, and probably work better than the initial state as well given it'd allow for more AST reuse.

[frodi-karlsson]

Thank you for both the patience and guidance, I'm very grateful to get so much feedback.

It seems quite difficult to decouple ImpliedNodeFormat from the SourceFile and into Program, as it's used in places where that wouldn't be available currently, such as commonjsmodule.go/shouldTransformImportCallWorker(vicariously through ast.GetEmitModuleFormatOfFileWorker) or in externalmoduleinfo.go/createExternalHelpersImportDeclarationIfNeeded.

I fear I may simply be in too deep water here, and that I should leave this task to the pros. Looking at the git blames, it looks like Ron Buckton might be working his way toward this problem, and I might just be making noise where there's a plan conceptualized somewhere anyway

[frodi-karlsson]

I'll close this PR, and keep keeping busy with smaller tasks until I have better understanding of the bigger picture, so as not to waste valuable review mana in these hectic times :)

[jakebailey]

I don't think you were too far off at all; I might try and resurrect this later, if you don't want to keep hacking on it.

[frodi-karlsson]

How motivating! I couldn't find any nice way to store it outside of the source file, but I think I managed to clean up the first "probably okay" version a bit, with the cache key fixed.

The diff is a bit less gratifying because I did reduce scope a bit into just ImpliedNodeFormat, as it should've been from the start

[rbuckton]

This looks good to me, assuming @andrewbranch and @jakebailey don't have any further feedback.

[jakebailey]

This lock is held for the entire call, so I don't think we're gaining any parallelism here, no?

The "fix" would be to only hold the lock for the check at the top, unlock it, then relock it at the bottom (moving the make call down too).

[jakebailey]

Which, also indicates that this should be a RWMutex.

[jakebailey]

Only some small notes. Very happy to see this nearly complete.

[jakebailey]

[andrewbranch]

Thanks!

[frodi-karlsson]

Thank you! I know I was a bit in over my head so it was a bit rocky, but I'll do my best to repay all these domain learnings to the repo with time