Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Diagnostic Error Codes #282

Merged
merged 13 commits into from
Apr 6, 2024

Conversation

Schottkyc137
Copy link
Contributor

@Schottkyc137 Schottkyc137 commented Apr 1, 2024

Motivation

There are a lot of issues about the general concept of allowing more message control (i.e., #255, #192, and some in the rust_hdl_vscode repository). This is an attempt at tackling the challenge of allowing message control of sorts. Once all is set and done, I envision a process where you can

  1. Disable or downgrade diagnostics in the vhdl_ls.toml, i.e.,
[libraries]
[lib1.files] = [...]

[config]
[error_codes]
# Downgrade all errors of the following kind to warnings
misplaced_attribute_spec = "warning"
  1. Disable or downgrade diagnostics in vhdl using the tool directive concept or plain comments:
entity foo is
    port (
        `vhdl_ls allow=unused
        bar: in bit;
        -- vhdl_ls allow=unused
        baz: out bit
    );
end entity;

(Note that the syntax for both examples is just for illustrative purposes and subject to change should these features be implemented).

This PR

This PR introduces error codes. Except for being forwarded to the language server and potentially being displayed, this is unused. However, I plan to add additional functionality that uses these error codes. This is to avoid error prone regex matching or similar when detecting an error that should be ignored or downgraded:

entity foo is
    port (
         -- Might work for now but matching on the text is potentially bad for readability and the actual message being displayed is subject to change
        `vhdl_ls disable="unused declaration pf port '*'"
        bar: in bit
    );
end entity;

I will leave this PR open for some time to collect some feedback, especially concerning how current errors without error codes are assigned to the different error codes. Later changes to the actual codes that are not purely additive (i.e., merging of two error codes, splitting an error code into multiple, ...) are not desirable as this will cause backwards incompatible changes. If no such feedback is given, I will assume that these error codes are a good start.

# Conflicts:
#	vhdl_lang/src/analysis/analyze.rs
#	vhdl_lang/src/analysis/association.rs
#	vhdl_lang/src/analysis/declarative.rs
#	vhdl_lang/src/analysis/design_unit.rs
#	vhdl_lang/src/analysis/expression.rs
#	vhdl_lang/src/analysis/names.rs
#	vhdl_lang/src/named_entity.rs
@kraigher
Copy link
Member

kraigher commented Apr 1, 2024

I think it is a good step to create error codes if we want to support user configurable error ignores.
I think it is very important that the error code is referred to by name and not by some magic number by the user. Just like in Rust with [allow(error_code)]. You could generate the error names by using the variant names of the rust enum, that will be very easy to maintain.

@Schottkyc137
Copy link
Contributor Author

Yea, from and to string are definitely planned. I'm not sure whether to use a custom string - to enum (and inverse) map or auto-deriving is better. The first approach has the advantage that the code representation of the enum is independent of what is seen by the user. One could, for example, easily change the name of the enum while at the same time being backwards compatible. But then on the other hand for packages like strum, such features are already present, I believe (by adding aliases for names). So I'm also heavily leaning towards auto generating names.

@kraigher
Copy link
Member

kraigher commented Apr 1, 2024

Auto generating names is probably the most maintainable. With strum you can have aliases for exceptions.

@Schottkyc137
Copy link
Contributor Author

Auto generating names is probably the most maintainable. With strum you can have aliases for exceptions.

I have included strum as a dependency to obtain error codes from a &str

@Schottkyc137
Copy link
Contributor Author

Probably there is little benefit now from having the numeric value being displayed. Currently, for example in VSCode, the error is displayed as "No declaration of ..." vhdl_ls(E038). But it's probably better to display it as "No declaration of ..." vhdl_ls(undeclared). Or do you see any benefits in keeping the numeric values, @kraigher?

@kraigher
Copy link
Member

kraigher commented Apr 1, 2024

Good systems for error codes do not use numbers. They add no value. A short name is both unique, self describing and can be looked up in a reference doc.

@Schottkyc137 Schottkyc137 mentioned this pull request Apr 1, 2024
1 task
@Schottkyc137 Schottkyc137 merged commit ce2d47a into VHDL-LS:master Apr 6, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants