Support for JS-interop errors in IDEs

[sigmundch]

Until now all JS-interop errors are reported by a CFE transformer that we use in ddc, dart2wasm, and dart2ms. These check the validity of certain JS-interop idioms and APIs.

We are reaching a level of maturity where it makes sense to start graduating these checks into more prominent errors that can be displayed in IDEs.

Options to do so are:

• include this as part of analyzer as an ad-hoc additoinl
• include this as a separate lint
• add these to the language spec, so they can be included as language checks for both the CFE and analyzer.

@srujzs - I'm curious of your initial thoughts here, before reaching out to the broader team?

[srujzs]

A few quick comments that I might add onto later:

• We definitely want either lints or analyzer errors for type-checks/casts that we know are bad for JS types. I still have to write a proposal here, but at the minimum I want to do some work here.
• I think in general, it's a good idea to show errors sooner rather than later if we can, but I'm not entirely sure every error we have can be migrated to the analyzer as well. Still, we can copy as many we can.
• How do we avoid too much duplication in the error-checking logic? I know the CFE and analyzer teams already deal with this with normal language errors.
• I'd want these to be a bit "flexible" for now until we reach further maturity. Ideally, it shouldn't be hard to change them as we're maturing, and encoding them into the language spec might make it too hard.

[bwilkerson]

A couple of thoughts in case they help with the decision-making process.

We definitely want either lints or analyzer errors for type-checks/casts that we know are bad for JS types.

The FFI team has it's own dedicated FfiVerifier class that they maintain in order to surface diagnostics related to the FFI library and package. The key there is that they maintain the class as the libraries are changed. That model has worked very well, and we'd be happy to have a JsInteropVerifier class (name negotiable) that was maintained by the dart2js team that does the same for JS-interop support.

... but I'm not entirely sure every error we have can be migrated to the analyzer as well.

With your own class you'd be in complete control of which diagnostics you implement.

How do we avoid too much duplication in the error-checking logic?

The analyzer and the front end teams have a shared package named _fe_analyzer_shared that contains the logic that the two teams share. That package has to be published because the analyzer package is published, but you could potentially use a similar model (or maybe even use the existing package).

That said, most of the code that's shared has to be written fairly abstractly because the front end and analyzer have distinct models for elements and types. For reasonably complex pieces like type inference the effort is worth it, but depending on how much logic could be shared it might or might not make sense. You might want to try duplicating a small portion of the logic to see how easy or hard it would be to share.

[srujzs]

Sorry for the late response - going through my backlog. :)

If we don't have the same errors in both the analyzer and the CFE, I think this would be better. I can imagine we move all the errors that can be moved into the analyzer and keep the rest in the CFE. Having the same errors in both will probably end up with a desync at some point due to small implementation differences, and I'd like to avoid that.

[sigmundch]

If we don't have the same errors in both the analyzer and the CFE, I think this would be better.

By "this" do you mean the Verifier class Brian proposed above? I'm supportive of that too :)

I can imagine we move all the errors that can be moved into the analyzer and keep the rest in the CFE.

Not sure about "moving", in that case. I'd expect we'd duplicate the errors instead since not all codepaths go through the analyzer first, we want to make sure a compilation via the CFE continues to report the errors as well.

[srujzs]

By "this" do you mean the Verifier class Brian proposed above?

Oh I meant "this" as not having the same errors, not anything about the verifier (which is a good resource).

Not sure about "moving", in that case. I'd expect we'd duplicate the errors instead since not all codepaths go through the analyzer first, we want to make sure a compilation via the CFE continues to report the errors as well.

Yeah, I figured we wouldn't be able to keep errors in the analyzer only. Maybe trying it out with one or two common errors might give me a better idea of maintenance cost/desync risk. I might be overestimating it.