Quick Fixes for Class Modifier Diagnostics

[bwilkerson]

There's only one diagnostic so far: CompileTimeErrorCode.INVALID_USE_OF_TYPE_OUTSIDE_LIBRARY (though there are 9 variations on the message for the sake of clarity).

Without knowing the specific semantics the user is trying to achieve, the only fixes I can think of are to

• change the type being used to one that's valid,
• remove the use of the type, or
• change the type being used so that its use is valid (add or remove a modifier).

I don't know how common any of those fixes are likely to be, so I don't know how useful any of these fixes would be to automate.

For the first, we have a fix that will look for similarly named types, though it doesn't know about the capability modifiers, so it might suggest a different class that's equally inappropriate. That limitation should be fixed for other reasons, even if we don't apply that fix here.

For the second, we could obviously remove the code.

For the third, we could update the modifiers appropriately (when the declaration of the type is in a file that's explicitly being analyzed), though there are likely to be a lot of cases to consider.

@kallentu In case you have ideas on this topic, including other diagnostics that still need to be added.

[eernstg]

Change the usage, if possible? (so extends C becomes implements ..., C if C is interface, and so on)

[bwilkerson]

Definitely an interesting possibility, thanks. I'm not sure how often those would be useful either, but then I don't know much about how best to help users around these errors.

[pq]

A fix to add @reopen when an implicit reopen (https://github.com/dart-lang/sdk/issues/58976) is desired seems reasonable.

[bwilkerson]

Agreed. I wasn't thinking about the lint that hasn't been added yet, just the existing diagnostics. :-)

[kallentu]

There's CLASS_USED_AS_MIXIN whose quick fix could be adding a mixin modifier.

And there will be another error diagnostic for the part of the spec that specifies A subtype of a base or final class must be base, final or sealed. but I have not added this yet. Perhaps the quick fix for this would be to add a base or final depending on the supertype's modifier. Not sure if that's the best heuristic, but I think we could potentially do something here?

Also, for what currently is MIXIN_INHERITS_FROM_NOT_OBJECT and MIXIN_CLASS_DECLARES_CONSTRUCTOR (which after talking to @bwilkerson, I'll make another one for Dart 3 to separate when the error occurs between old versions and >3) -- we can make a fix that removes the inheriting or removes the constructor respectively.

For INVALID_USE_OF_TYPE_OUTSIDE_LIBRARY, I was talking about this briefly with @pq yesterday and I couldn't figure out a good quick fix. You can't remove the modifier from the class declaring it, because it seems like that's the intention of the author to add a restricting modifier, otherwise they would've left it as class. I also don't know how useful changing the type of inheritance (extends to implements for example) would be. Removing the subtyping relation would be okay.

[bwilkerson]

We didn't create issues or even a check list for any of these ideas. Do we know of any fixes that would be particularly valuable?

[kallentu]

MIXIN_CLASS_DECLARATION_EXTENDS_NOT_OBJECT -- removing the extends __ or with __. CLASS_USED_AS_MIXIN -- adding a mixin modifier SUBTYPE_OF_BASE_IS_NOT_BASE_FINAL_OR_SEALED -- add base onto the erroneous subtype SUBTYPE_OF_FINAL_IS_NOT_BASE_FINAL_OR_SEALED -- add final onto the erroneous subtype

Maybe these ones are low hanging fruit?

Although, people will encounter INVALID_USE_OF_TYPE_OUTSIDE_LIBRARY the most probably.

[bwilkerson]

Maybe these ones are low hanging fruit?

I don't know. It's hard for me to guess at (a) how often users will run into these errors and (b) whether these fixes are what users are going to want to do to fix the error.

All of these suggestions are reasonable ways to fix the error, I just don't know whether it's worth the cost for us to implement them. I wouldn't object to having them, I just don't want to ask anyone to go to heroic lengths to get them implemented before the deadline.

Below is an attempt to evaluate the value of each, but I don't claim to have all the information necessary to make a good choice.

If we decide to not do some or all of these, it would be good to capture the ideas in the error_fix_status.yaml file so that we don't lose them.

MIXIN_CLASS_DECLARATION_EXTENDS_NOT_OBJECT -- removing the extends __ or with __.

Or move the class to the implements clause? Or replace it with a different type altogether? Or use extension methods? Or completely redesign their implementation?

It's kind of hard to know what the right solution is without more context than we can get from static analysis. My inclination at this point would be to not offer any of these fixes because I don't know that they'd bring enough value to users.

CLASS_USED_AS_MIXIN -- adding a mixin modifier

This one sounds more promising, though I don't know how often this will occur when the class being used as a mixin is in the same package as the use site (that is, somewhere we can edit). I'd probably pass on this one too because I wouldn't expect it to be a common issue. (We generally avoid fixes that require making non-local changes for exactly this reason.)

SUBTYPE_OF_BASE_IS_NOT_BASE_FINAL_OR_SEALED -- add base onto the erroneous subtype SUBTYPE_OF_FINAL_IS_NOT_BASE_FINAL_OR_SEALED -- add final onto the erroneous subtype

Or sealed? I'm guessing that both of these would require all three possible fixes in order to be complete, though it's possible that adding the modifier from the supertype is the right solution in the majority of cases. Again, it's hard to know whether this fix / these fixes would add enough value to justify the cost.

INVALID_USE_OF_TYPE_OUTSIDE_LIBRARY

Not sure what the fix would be for this one.