Migrate lints to analysis server plugin

[PiotrRogulski]

In separate PRs:

• re-write tests
• move implementation to src

What's not yet available:

• use_design_system_item: separate lint rules per config entry
• prefer_center_over_align: our current implementation requires an asynchronous resolution of another library, which isn't (yet?) possible

(cc @shilangyu, I can't add you as a reviewer 🙈)

[PiotrRogulski]

Actually, I might need to include the tests in this PR, since the check is complaining

[shilangyu]

What is your opinion on how the tests look like now?

To me, they seem not maintainable and not made for humans to edit. I also do not like that we are mocking libraries rather than testing on the real thing.

If you think the previous testing framework was better, I can try to write something that will make it work (if I find time). We can improve on what custom lint did and expect lints in some specific range rather than in the next line. Either way, I guess this is not mergable until we figure out how to support configs (which are crucial)? There could be a gradual migration of lints that don't need custom_lint features but still stay with custom_lint for the rest. What do you think (or, what is LeanCode's roadmap for leancode_lint here)?

[PiotrRogulski]

What is your opinion on how the tests look like now?

Not particularly fond of them, honestly 😅

they seem not maintainable and not made for humans to edit

That's not a huge issue; the testing tooling suggests appropriate locations:

Found but did not expect:
  use_design_system_item.use_design_system_item [57, 8, Scaffold is forbidden within this design system., "Use the alternative defined in the design system: LftScaffold."]
  use_design_system_item.use_design_system_item [106, 4, Text is forbidden within this design system., "Use the alternative defined in the design system: LftText."]

To accept the current state, expect:
  lint(57, 8),
  lint(106, 4),

Either way, I guess this is not mergable until we figure out how to support configs (which are crucial)

I mean, the config, even if not done "officially", already works

[shilangyu]

That's not a huge issue; the testing tooling suggests appropriate locations:

Found but did not expect:
  use_design_system_item.use_design_system_item [57, 8, Scaffold is forbidden within this design system., "Use the alternative defined in the design system: LftScaffold."]
  use_design_system_item.use_design_system_item [106, 4, Text is forbidden within this design system., "Use the alternative defined in the design system: LftText."]

To accept the current state, expect:
  lint(57, 8),
  lint(106, 4),

I don't see this being useful. 57, 106 means nothing to me, I don't know which range this is indicating

[PiotrRogulski]

That's not a huge issue; the testing tooling suggests appropriate locations:

Found but did not expect:
  use_design_system_item.use_design_system_item [57, 8, Scaffold is forbidden within this design system., "Use the alternative defined in the design system: LftScaffold."]
  use_design_system_item.use_design_system_item [106, 4, Text is forbidden within this design system., "Use the alternative defined in the design system: LftText."]

To accept the current state, expect:
  lint(57, 8),
  lint(106, 4),

I don't see this being useful. 57, 106 means nothing to me, I don't know which range this is indicating

Yes, in this regard the experience is very poor. I just end up selecting text in the editor and seeing how many characters I select 🙈

[PiotrRogulski]

@shilangyu Actually, I'll try refactoring the tests to use TestCode and range-based markup (dart-lang/sdk#61889, maybe someday it'll be oficially supported; for now it works with a src import)

[shilangyu]

Nice! Thanks. I have started reviewing this PR btw. Just not done yet

[shilangyu]

Thank you for this huge effort! This is amazing. It is nice to finally get first party support in dart.

[shilangyu]

This unfortunately reads only analysis_options from the root of the workspace. It is an important feature (and used at LeanCode) that configs are scoped to some directory.

package:analyzer/src/analysis_options handles this. See the AnalysisOptionsProvider class. It also handles merging options from upper configs and resolves include: directives.

[shilangyu]

I don't think this method does merging of include:s. getOptions did.

[PiotrRogulski]

U right. getOptions doesn't seem to behave well in tests (it returns a map with all the correct keys, but with null values: {analyzer: null, linter: null, leancode_lint: null} 🤔) Will have to investigate

[PiotrRogulski]

Something weird is going on when combining yaml maps :/

Before merging – everything looks ok:

After merging – values are null 😔

[shilangyu]

I will have a look at it by the end of the week, thanks

[shilangyu]

I spent some time debugging this. It really seems like a bug. I tried to look at how the SDK uses this API but found nothing jarring. I hope to find more time this week (but it is very busy for me) to resolve this finally.

[shilangyu]

What I gathered from the SDK team responses is that there is no good way to resolve this. I think we need to give up on computeConstantValue approach and just hardcode some cases. Or, I can try the proposed approach of crawling from the Align widget to the Alignment.center constant. I wonder how expensive this is.