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

stage2: a decl needs to have multiple compile errors attached to it #7218

Closed
g-w1 opened this issue Nov 25, 2020 · 7 comments
Closed

stage2: a decl needs to have multiple compile errors attached to it #7218

g-w1 opened this issue Nov 25, 2020 · 7 comments
Labels
frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Milestone

Comments

@g-w1
Copy link
Contributor

g-w1 commented Nov 25, 2020

Right now in Module.zig the failed_decls field of Module is defined like this

failed_decls: std.AutoArrayHashMapUnmanaged(*Decl, *Compilation.ErrorMsg) = .{},

1 decl points to 1 error message. I can think of at least one use case where more than one error is needed. In @compilelog it gives a compile error, but doesn't stop the analysis. This means that we need somewhere to put multiple compile log errors, errors that don't fail a Decl, so more can be generated.

I tried to impliment this with a Decl pointing to and arraylist of Compilation.ErrorMsg, but I had to change a lot of things and it is using unnecessary memory for an arraylist because most errors will only be single.

My question is if there is another use/possibility for compile errors that don't stop analysis. If not, I think the best thing to do would be to make a separate field on Module for failed_nonstopping_decls or compile_log_decls with Decl -> ArrayList(ErrorMsg) so that we save memory.

This seems necessary for me to continue my compileLog pull request.

Edit: another use case is a switch on an error that doesn't cover all possibilities:

./test.zig:4:5: error: error.A not handled in switch
    switch (a) {
./test.zig:4:5: error: error.C not handled in switch
    switch (a) {
@Vexu Vexu added the frontend Tokenization, parsing, AstGen, Sema, and Liveness. label Nov 27, 2020
@Vexu Vexu added this to the 0.8.0 milestone Nov 27, 2020
g-w1 added a commit to g-w1/zig that referenced this issue Dec 24, 2020
@andrewrk
Copy link
Member

andrewrk commented Dec 26, 2020

Sorry I meant to reply to this earlier. I disagree with decls having more than 1 compile errors attached to them. I think the compiler should stop trying to analyze a Decl once it hits a single compile error for it.

Furthermore, when a decl is referenced that has a compile error attached to it, that should return a "poison" value that indicates to stop analysis for the decl that is referencing the erroneous one.

1 compile error per Decl is plenty for the programmer to handle, and keeps the compiler implementation simpler, with less memory usage.

For the compile log error, it should just be 1 global error for the entire Module, with hints pointing at the log locations. (or maybe just 1 hint pointing at the first one)

@Vexu
Copy link
Member

Vexu commented Dec 26, 2020

What about errors that have no effect on behavior like those for unused labels/variables?

I don't think we should be spending too much time optimizing error note memory usage since they are supposed to be rare.

@andrewrk
Copy link
Member

andrewrk commented Dec 26, 2020

This is primarily about avoiding redundant compile errors for users, and compiler implementation simplicity. In C++ for example, the workflow is to fix the first compile error and then rebuild. In Zig the programmer should feel like fixing all the compile errors before doing another rebuild would be worth their time.

Good point about errors that have no effect on behavior; because of this I think it makes sense to have the poison state either stored separately or inferred from other state rather than based on existence of a compile error in the table. For example it may make sense to have a Type that corresponds to "poison", e.g. any reference or use of this Type silently fails compilation (the compile error has already been emitted separately). In the interest of incremental builds, it might be necessary for a poison value to store a reference to a set of dependencies so that if any of the dependencies are modified then the Decl containing the poison reference would get re-analyzed.

@g-w1
Copy link
Contributor Author

g-w1 commented Dec 26, 2020

I do not know if you are aware, but this got added in 1634d45 , so I was going to close it, but this is a good discussion to have.

I personally like the workflow of the compiler spitting out all the errors and then going through them one-by-one and fixing them. I've heard of this called compiler driven development.

For the idea about having a Module-level compile log list, that is basically what I did here, 3a7c08e#diff-48982308f6213afa65404cb32e773d7c8d252bbc21fe2724a96c103baf6cb853R58 , but then got told here #7239 (comment) to just use a global Decl -> List(Error).

I would not like it if the compiler told me that I was missing one switch case, and then recompiled and had to implement another switch case. We could of course make the error, "missing switch cases" and then have notes to which ones are missing, but I think that in the future, more and more cases like this will pop-up and it will be sub-optimal to find workarounds.

@andrewrk
Copy link
Member

I personally like the workflow of the compiler spitting out all the errors and then going through them one-by-one and fixing them. I've heard of this called compiler driven development.

To be clear, I am arguing in favor of this workflow. For this workflow it is important to not have errors that are red herrings; that is, generally, fixing one compile error should not cause a different compile error to also become fixed. This way the programmer can trust that it will be worth their time to look at all the compile errors before attempting to recompile.

I would not like it if the compiler told me that I was missing one switch case, and then recompiled and had to implement another switch case. We could of course make the error, "missing switch cases" and then have notes to which ones are missing, but I think that in the future, more and more cases like this will pop-up and it will be sub-optimal to find workarounds.

I don't see this way of organizing compile errors as a workaround but as the most sensible way to do it. stage1 currently emits an error for each missing case but I think it should be the "missing switch cases" with each one as a note, as you said. Can you think of a real example of a situation where it would be valuable to have multiple errors (not notes) in a single Decl?

Anyway, it's ok to leave things how they are, it's on me to do the work of changing things at this point :)

@marler8997
Copy link
Contributor

The mechanism @andrewrk describes about poison values reminds me of Walter's explanation on how the D compiler handles errors. You can watch his comments here: https://www.youtube.com/watch?v=bNJhtKPugSQ&t=45m56s

IMO there is a balance to be struck between too much and not enough information with compile errors. I think the poison mechanism is a nice compromise between the 2 extremes.

@Vexu
Copy link
Member

Vexu commented Dec 26, 2020

Having multiple errors per decl also makes error notes nice and simple: #7555.

g-w1 added a commit to g-w1/zig that referenced this issue Dec 26, 2020
g-w1 added a commit to g-w1/zig that referenced this issue Dec 29, 2020
g-w1 added a commit to g-w1/zig that referenced this issue Dec 29, 2020
g-w1 added a commit to g-w1/zig that referenced this issue Dec 29, 2020
…, NOTE: this will fail the ci"

This reverts commit ef616e4.
@andrewrk andrewrk added the proposal This issue suggests modifications. If it also has the "accepted" label then it is planned. label Jan 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
frontend Tokenization, parsing, AstGen, Sema, and Liveness. proposal This issue suggests modifications. If it also has the "accepted" label then it is planned.
Projects
None yet
Development

No branches or pull requests

4 participants