There are too many ways to toggle diagnostics

[carsakiller]

Why is there 3 different ways to disable diagnostics?

The Problem

Currently, there is:

• Lua.diagnostics.disable
  • An array of strings where you specify each diagnostic to disable
  • Simple, but has no control for open/closed files
• Lua.diagnostics.neededFileStatus
  • An object where the key is the name of the diagnostic and the value is how the diagnostic will be checked (not at all, only open files, all files)
  • Slow but precise, allows you to alter the most diagnostics but can be slow to get everything set up the way you like it. Good for long-term setup where you can get set up perfectly.
• Lua.diagnostics.groupFileStatus
  • An object where the key is the name of the group and the value is how the group of diagnostics will be checked (not at all, only open files, all files)
  • Quick but not precise, allows the user to quickly turn off entire categories of diagnostics but does not allow for disabling individual diagnostics. Good for short-term setup.

This can get very confusing very quickly.

The solution

I think going with just Lua.diagnostics.neededFileStatus would make the most sense.

It allows for the user to completely customize how the server runs, which is more important than the user quickly being able to get things working sort of how they like it in the short-term.

To still help get users up and running as fast as possible, we can offer "presets" or "popular configs" on the discussions page. This can help satisfy those that want just the basics or full strict mode.

This would definitely be a breaking change but I think it is a very important one to make!

Originally posted by @carsakiller in #1362 (comment)

[sumneko]

Lua.diagnostics.disable needs to remain compatible and is used for quick fixes.
Lua.diagnostics.neededFileStatus for advanced users to customize diagnostics. And some diagnostics with side effects will use this setting to be turned off by default (disable as an array can't do that).

[carsakiller]

Is there a reason why Lua.diagnostics.disable has to remain compatible? I think the naming is poor too as there is a Lua.diagnostics.enable that is not for enabling each diagnostic but for toggling all diagnostics at once, which makes more sense.

I wouldn't consider Lua.diagnostics.neededFileStatus an advanced feature (although a rename may be needed), but rather the best way to toggle diagnostics as you have the most control over them. It is not really any slower than Lua.diagnostics.disable either, as instead of adding the diagnostic to an array as a string, you are just adding it to an object with the value of "None".

If Lua.diagnostics.neededFileStatus can do everything the other settings can do with no drawback other than maybe it is a bit slower, I see no reason to keep the others. If a user wants to quickly disable a certain diagnostic everywhere, then they could perform a "quick fix" that adds "<diagnostic>": "None" to Lua.diagnostics.neededFileStatus.

[sumneko]

Is there a reason why Lua.diagnostics.disable has to remain compatible? I think the naming is poor too as there is a Lua.diagnostics.enable that is not for enabling each diagnostic but for toggling all diagnostics at once, which makes more sense.

Lua.diagnostics.disable is one of the earliest settings. Lua.diagnostics.enable is to keep it consistent with other similar settings like Lua.hover.enable.

For me, all the two settings can be accepted. I did not modify it just to keep compatible.

[carsakiller]

I understand that you want to keep it backwards compatible, but I think having three settings to enable/disable diagnostics (Lua.diagnostics.disable, Lua.diagnostics.groupFileStatus, Lua.diagnostics.neededFileStatus) severely hinders the usability of the settings.

For example, I have used this extension in VS Code for over a year at this point and I had no idea that the await group was disabled by default. I didn't know it was a feature, and when I did, I didn't know how to enable it until reading the default value for Lua.diagnostics.neededFileStatus. Part of this problem can be fixed with better documentation in the wiki, but the main root of the problem is that there are 3 settings with different names that all accomplish something very similar.

I think it is needlessly complex and is very confusing to new users.

Having one setting called Lua.diagnostics.config or something similar that functions like Lua.diagnostics.neededFileStatus would be a lot easier to understand and document.

[sumneko]

I think I can remove disable and rename neededFileStatus to config, rename groupFileStatus to groupConfig

[carsakiller]

I still think the groups add extra complexity for both developers and users.

• Developers have to always remember to add new diagnostics to a fitting group or create a new group for just one setting (see redefined, strong).
• Users have to understand which diagnostics fall under which group, and to enable something they have to still check 2 settings (groupConfig and config).

This added complexity only serves to make configuring a little faster, assuming that changing an entire group of settings is actually what you want to do. Personally, I would never use the groups setting and only enable/disable the diagnostic I actually want to change, but maybe we get some more opinions?

[carsakiller]

@Nexela @FlashHit @Rouneq @sewbacca @flrgh @lua-rocks

Sorry for the mass mention if you are not interested, but I noticed you guys are pretty active on this repository and I think your opinions would be very valuable here as this is a pretty substantial change.

Please take a read through to get an understanding of the issue and then, if you could, provide your opinion on the situation. I just want to make sure I am not suggesting a major change that does not resonate with a lot of the community.

Thank you 🙂.

[Rouneq]

If I understand both sides of the discussion correctly, my main question would be: Is there a benefit to having so many different means of accomplishing the same task? For example, what's the driving use case for groupFileStatus? I think I understand what it does. Why was it put in to do what it does? Was there a call to disable whole groups of diagnostics at some point?

As for diagnostic vs needFileStatus, do I understand correctly it's just a matter of specifying

"Lua.diagnostics.disable": [
    "<diagnostic.1>",
    "<diagnostic.2>"
]

or

"Lua.diagnostics.neededFileStatus": {
    "<diagnostic.1>": "None",
    "<diagnostic.2>": "None",
}

[carsakiller]

@Rouneq I believe that diagnostics.groupFileStatus was meant to make it faster to disable many diagnostics. For example, disable all type checking by setting the type group to "None". I have not found any issue asking for this feature.

diagnostics.disable is short and simple, you enter the name of the diagnostic, it is disabled.

diagnostics.neededFileStatus allows you to set when each diagnostic is in effect. So if you only want to be warned about lowercase-global in files that are opened, you set lowercase-global to "Opened". If you want to disable undefined-global (which is in the same "group" as lowercase-global) you set it to "None". This functionality is not available through the other two settings.

My point is that diagnostics.neededFileStatus provides the most control, so it should be the only necessary setting, the others are just causing confusion and should be removed. For those that don't want to go through each individual setting, they could maybe find a configuration that works for them on the discussions page or wiki, or just stick with the default.

EDIT: I think of it like a linter config like ESLint, yes there are a lot of rules, but it lets you dial things in exactly how you want it.

[Rouneq]

Right. groupFileStatus seems like a convenience mechanism for internal development of the server. Making it available to end-users feels like a bonus. Sure, I could disable all type diagnostics, but is there a need for someone to do so? (Please don't fixate on the type group specifically. This is a general question.) I can't think of a driving need for the capability other than convenience. But I'll admit I doubt I'm the intended audience for it.

[carsakiller]

My main concern with the groups is that you now have the toggling of diagnostics spread out across two settings. The group can also be specified as a "Fallback", which means its true state is set by diagnostics.neededFileStatus. This can lead to confusion, like in the example I provided where the async group isn't actually disabled by diagnostics.groupFileStatus, but because it is set to "Fallback", is actually individually disabled by diagnostics.neededFileStatus.

I haven't even gotten into the fact that diagnostics.neededFileStatus can override diagnostics.groupFileStatus by appending a ! to the end of the value specified. At that point you are already using diagnostics.neededFileStatus but overriding previous settings, adding to the confusion.

[carsakiller]

I think the groups can still be made to work, if there is a need for it, however, I don't see much of a need for it in the first place. It would also make sharing configs a bit harder as you would be sharing two settings instead of just one object.

I think it could work like this:

1. Set the state for a group

"Lua.diagnostics.groupFileStatus": {
    "global": "None"
}

2. Anything not defined in Lua.diagnostics.groupFileStatus automatically falls back to Lua.diagnostics.neededFileStatus
3. Set the state for lowercase-global because I want that enabled (but not the other global diagnostics) which automatically overrides Lua.diagnostics.groupFileStatus

"Lua.diagnostics.neededFileStatus": {
    "lowercase-global": "Any"
}

4. global-in-nil-env, undefined-env-child, undefined-global are disabled, lowercase-global is the only diagnostic from the global group that is enabled.

[Nexela]

I think it could work like this:

I also think this is the best way. With the removal of the ! which just adds confusion to new users.

[flrgh]

I don't have a very strong opinion about this myself, so please don't take any of this as much more than a couple naive ideas.

Here are my thoughts around making the config more expressive and succinct by combining diagnostic groups with diagnostic names/identifiers, as well as allowing simple operator syntax like globbing and numeric.

Note: I'm staying away from commenting on config names like Lua.diagnostics.neededFileStatus for now, so just read diagnostics as a placeholder in the examples below. I'm also not commenting on the setting enum (None/Any/Opened/Fallback) for the moment, so don't read into that too much either.

diagnostics = {
  -- groups and individual diagnostics both go in the same table using some
  -- notation to separate them (probably "." or ":")

  -- disabling a diagnostic using the fully-qualified (includes the group name) identifier
  ["type-check:need-check-nil"] = "None", 

  -- although groups _look like_ namespaces, they're just tags/labels, so diagnostic names
  -- must be globally unique such that this is equivalent to the previous line
  ["need-check-nil"] = "None",

  -- disabling all checks in a group by using globbing syntax
  ["ambiguity:*"] = "None",

  -- setting an entire group's semantics using globbing syntax
  ["luadoc:*"] = "Opened",

  -- disabling an entire group except for one specific diagnostic
  --
  -- requires some sort of formalized resolution order to work properly
   -- (i.e. "Any" is resolved _after_ "None")
  ["duplicate:*"] = "None",
  ["duplicate:duplicate-set-field"] = "Any",
}

Resolution order can be a problem with expressing intent succinctly, and I don't have a good solution for that. Older applications often allow the user to specify the resolution order (i.e. Apache's mod_access "Order" option), which is very useful, but that does introduce another configuration field (though maybe it's worth it if this more expressive format could eliminate several other config fields).

Assigning and exposing diagnostic codes to diagnostics (this might even be the case already in the code--I haven't looked at it) would open up the door for more expressive syntax using ranges or numeric filters. The luacheck warnings are laid out like this. The first digit of the error code is a group/class of error, which feels intuitive to me, similar to HTTP codes (HTTP 4xx are client errors, HTTP 5xx are server errors).

diagnostics = {
  -- disable all 0xx, 1xx, and 2xx diagnostics
  ["<300"] = "None",

  -- alternatively, using range notation
  ["0..299"] = "None",

  -- disable diagnostic with code 412
  ["412"] = "None",

  -- disable all 5xx diagnostics except 554
  ["500..599"] = "None",
  ["554"] = "Any",
}

I think it would be good to look at luacheck for further inspiration, as most of what I've presented here is copied from it (i.e. globbing/pattern-matching errors). It's likely that a users lua-language-server is also familiar with luacheck, so using a configuration scheme that is similar would make things intuitive.

[carsakiller]

I do like that glob pattern idea, that way we can keep the groups but only need one setting - best of both worlds! That is the kind of solution I was looking for 😁.

As for resolution order, I think something like this could do:

local config = {
	["type-check:*"] = "None",
	["need-check-nil"] = "Opened",

	["duplicate:duplicate-set-field"] = "Any",
	["duplicate:*"] = "None",
}

local keys = {}

for k, v in pairs(config) do
	table.insert(keys, k)
end

table.sort(keys, function(a, b)
	return (string.find(a, "*")) and not (string.find(b, "*"))
end)

-- keys are now sorted so all glob patterns are at the front
-- {type-check:*, duplicate:*, duplicate:duplicate-set-field, need-check-nil}

This way all glob patterns are applied first and and then more specific settings are applied afterwards. We start broad and narrow in, allowing people to override group settings for an individual diagnostic.

---

Not really a fan of the numerical codes though, it goes from a system where the diagnostic "IDs" go from pretty intuitive names like diagnostics.disable to something that would require a lot of experience using and a cheat sheet... rather than just being able to read.

[Nexela]

I do like the idea suggested by flrgh As well as sorting idea by carsakiller

"*": "opened" does seem a lot easier than my current way of opening another workspace so I can copy a massive block of settings

[flrgh]

As an amendment to my earlier comment, I think the groups concept could be made irrelevant if diagnostic/error names were consistently named with a prefix.

I.E. instead of this:

{
  duplicate = {
    "global-in-nil-env",
    "lowercase-global",
    -- ...
  },
  global = {
    "circle-doc-class",
    "doc-field-no-class",
    -- ...
  },
}

...this:

{
  "duplicate-global-in-nil-env",
  -- alternatively, with `:` as a separator instead of `-`
  "duplicate:lowercase-global",
  -- ...
  "global-circle-doc-class",
  "global-doc-field-no-class",
  -- ...
}

This still jives with being able to use glob syntax to enable/disable one or many diagnostics easily, and I think it's more intuitive than having to look up which "group" a diagnostic belongs to. The config parser need not to be aware of the group concept either. It's just handling strings and glob patterns, so duplicate-* is valid, but duplicate-global-* is also valid even though it might only match one diagnostic name.

Also, no added confusion about type-check:need-check-nil and need-check-nil being equivalent references to the same thing. The new name would simply be type-check-need-check-nil/type-check:need-check-nil.