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

false STRING assignment leads to panicked #1168

Open
rarris opened this issue Mar 20, 2024 · 13 comments
Open

false STRING assignment leads to panicked #1168

rarris opened this issue Mar 20, 2024 · 13 comments
Labels
bug Something isn't working

Comments

@rarris
Copy link
Contributor

rarris commented Mar 20, 2024

the following code

PROGRAM mainProg
VAR
  x3 : STRING;
END_VAR
  x3 := 11;
END_PROGRAM

will generate the following error with panicked:

mainProg.st:5:1:{5:1-5:9}: warning[E037]: Invalid assignment: cannot assign 'DINT' to 'STRING'
thread '' panicked at src\codegen\llvm_typesystem.rs:187:18:
internal error: entered unreachable code: Cannot cast integer value to STRING
note: run with RUST_BACKTRACE=1 environment variable to display a backtrace

@rarris rarris added the bug Something isn't working label Mar 20, 2024
@mhasel
Copy link
Member

mhasel commented Mar 20, 2024

This seems to be a diagnostics severity problem. Invalid assignment should be an error, not a warning - then we also would not enter codegen and no panic would be happening.

E037 should default to Severity::Error - are you using a custom config?

@volsa
Copy link
Member

volsa commented Mar 21, 2024

Closing this because by default it's an error as can be seen here; feel free to re-open if it's not the case @rarris

@volsa volsa closed this as completed Mar 21, 2024
@rarris
Copy link
Contributor Author

rarris commented Mar 22, 2024

here is about the panicked text, not the error itself
we should solve/remove all the panicked errors output text!

@mhasel
E037 should default to Severity::Error - are you using a custom config?

yes

@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

Reopening because we should not reach a panic state.

@ghaith ghaith reopened this Mar 22, 2024
@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

This seems to be a diagnostics severity problem. Invalid assignment should be an error, not a warning - then we also would not enter codegen and no panic would be happening.

E037 should default to Severity::Error - are you using a custom config?

I still think we should not panic but exist gracefully although this is a bigger topic. Should we make some errors non-changeable like the E37 or the one for references? This used to be the case with the critical errors

@volsa
Copy link
Member

volsa commented Mar 22, 2024

Should we make some errors non-changeable like the E37 or the one for references?

Big fan of this, any crucial error code should have an immutable severity imo

@tisis2
Copy link

tisis2 commented Mar 22, 2024

how would the user be able to recognize which error codes are configurable and which are not?
it feels strange for me when there is no recognizable difference for the user between the errors but if the id is put in the configuration as warning nothing happens. it would feel buggy then imo

@rarris
Copy link
Contributor Author

rarris commented Mar 22, 2024

this is a very good point @tisis2
we should have 2 error lists/categories
if a error is norm defined error and codesys error, that cannot be configured!

@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

The way rust does this is they have 2 types of errors, hard errors that cannot be changed, these are the E ones, and then named errors that can be changed in severity. These are ones things like deprecation warnings or clippy issues, they call them lints. https://rustc-dev-guide.rust-lang.org/diagnostics.html
We can try going that route.

@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

this is a very good point @tisis2 we should have 2 error lists/categories if a error is norm defined error and codesys error, that cannot be configured!

I would say if an error can lead to a panic, it's non configurable. Things like this error here or the reference error.

@ghaith
Copy link
Collaborator

ghaith commented Mar 22, 2024

But how do we show this? Do we make these errors have different codes? And give out an error if the error_config.json ignores one of them? We could introduce a "critical" section to the registry, and on loading a custom error config, if any of the critical errors gets a reduced severity we through an error and don't compile.

@tisis2
Copy link

tisis2 commented Mar 22, 2024

maybe something like that... we could also reflect it in the error id and differ between CRIT... and E... for example.

but to be honest, i think currently all this is nice to have... if a user changes an error to a warning and gets a panic after that step, the user might recognize that it is about the configuration that was changed...

@rarris
Copy link
Contributor Author

rarris commented Mar 22, 2024

we can make a template where only at certain error codes can the severity be changed

for example:

{
    "not configurable": [
        E037,
        E....
        E....
     ],
    "warning": [
     ],
    "ignore": [
     ]
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants