Missing JSDoc for assignment expression statement

[jackbsteinberg]

Currently if a variable is reassigned to a new type:

/*
 * param {Node} arg
 */
function f(arg) {
  if (something) {
    // reassigns arg from Node to Element
    arg = arg.firstElementChild;
    arg.localName;
  }
}

the rewriter will not know the type was reassigned, and localName would be rewritten by an import from "std:global/Node".

We could counteract this by adding a more sophisticated check, for both variable declarations and assignment expression statements, or we could add a rule to the README to not reassign a variable to a new type.

[fergald]

If we add a rule to the README we should also throw an exception if we detect the rule being broken rather than output code we know to be incorrect.

Should that example be

/*
 * param {Node} arg
 */
function f(arg) {
  if (something) {
    // reassigns arg from Node to Element
    arg = arg.firstElementChild;
  }
  arg.localName;
}

if not, we should consider this example too. Is there even a correct way to rewrite this? In theory node_local_name and element_local_name could be different in practice they are the say.

@domenic is it allowed to override methods in base classes for builtins? I guess there's no need to since that dispatch logic can be built into the C++ code.

[domenic]

If we add a rule to the README we should also throw an exception if we detect the rule being broken rather than output code we know to be incorrect.

Although this seems like a relative nice-to-have, I don't think we should duplicate the functionality of ESLint in this repository. So if we were to want to go this route, then I think we should just build a second tool that's a frontend in front of this tool plus ESLint.

Is there even a correct way to rewrite this?

There is no correct way to rewrite this modified example. Node does not have a local name.

is it allowed to override methods in base classes for builtins? I guess there's no need to since that dispatch logic can be built into the C++ code.

Although, as you note, it's generally not necessary, it's definitely possible to have different method definitions on subclasses than on the superclass.

[fergald]

If we add a rule to the README we should also throw an exception if we detect the rule being broken rather than output code we know to be incorrect.

Although this seems like a relative nice-to-have, I don't think we should duplicate the functionality of ESLint in this repository. So if we were to want to go this route, then I think we should just build a second tool that's a frontend in front of this tool plus ESLint.

I'm thinking of a different case, I think. I filed #76 for what I'm thinking about.

In Jack's example, we're not going to do something unsafe, right? We're going to rewrite the .localname and the code will fail at runtime if the instance ends up being incompatible with the original we chose. So I'm OK with this example not being rejected. The example in comment 1 does not get rewritten at all (filed #75)

In general though, for this to be a safe tool for production use, it has to reject the cases it knows it cannot fix. It doesn't make sense to leave that to another tool because that tool has to have a deep understanding of the rewriter in order to know what the rewriter can't fix. It's easier and more likely correct to add raise ... in some else clauses in the rewriter than to try to reconstruct the logic in some other tool. Some of the patterns that we need to avoid are not lint in "normal" JS but they are unsupportable in LAPI JS.

Is there even a correct way to rewrite this?

There is no correct way to rewrite this modified example. Node does not have a local name.

There's currently no valid example that I can find.

is it allowed to override methods in base classes for builtins? I guess there's no need to since that dispatch logic can be built into the C++ code.

Although, as you note, it's generally not necessary, it's definitely possible to have different method definitions on subclasses than on the superclass.

From what I recall, in blink at least, we have chosen to have only 1 method, have that in node.cc and have it check the type and do something different if it's an Element etc and so we can just pick the basest class and use the original from that but that seems to just have been luck (or maybe an attempt to minimize the number of bidings and maybe that it's faster to check Node vs Element in C++ than in V8?).

[domenic]

In general though, for this to be a safe tool for production use, it has to reject the cases it knows it cannot fix.

Although I think we're not very far apart on the specifics, I fundamentally disagree with this general principle. I think it is OK for tools to be imperfect, and I think it's especially OK when you can combine those tools with other tools to improve their coverage.

From what I recall, in blink at least, we have chosen to have only 1 method

This is just following the specs. The Blink implementation is not relevant here. The specs say that e.g. nodeName is defined on Node, and do not say that it is defined on Element, so that's what shows up to JavaScript.

The opposite case can also appear in the specs, although more rarely. For example toJSON appears explicitly on both PerforamnceLongTaskTiming and on its base class, PerformanceEntry.