Skip to content

Restore model validation not to fail on diagnostic errors for rules and scripts - #5351

Merged
kaikreuzer merged 6 commits into
openhab:mainfrom
jimtng:model-error-handling
Feb 14, 2026
Merged

Restore model validation not to fail on diagnostic errors for rules and scripts#5351
kaikreuzer merged 6 commits into
openhab:mainfrom
jimtng:model-error-handling

Conversation

@jimtng

@jimtng jimtng commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng
jimtng requested a review from a team as a code owner February 13, 2026 16:43
@openhab-bot

Copy link
Copy Markdown
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rules-dsl-in-5-1-is-now-unloading-rules-if-it-has-unreachable-expressions/168382/15

@lolodomo

Copy link
Copy Markdown
Contributor

We should restore only for DSL rules and not for other DSL stuff. If not, you will break what you have done

@jimtng

jimtng commented Feb 13, 2026

Copy link
Copy Markdown
Contributor Author

We should restore only for DSL rules and not for other DSL stuff. If not, you will break what you have done

Do you happen to remember why we had to be strict? At the very least, if we do need to be strict, we need to add a test that would fail when it isn't (like when it's changed in this PR)

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng

jimtng commented Feb 13, 2026

Copy link
Copy Markdown
Contributor Author

Before #4928

  • An error in one item in an .items file will not cause the entire file to be rejected.

We thought that being strict was a good thing, but it may not be.

This PR restores the error "tolerance" that we used to have pre-5.1.

Syntax errors still cause the whole file to be rejected.
However, "inconsistencies" in one item/thing/rule will not cause the whole file to be discarded.

The only other thing we should probably change is the logging should not be info. It should be a warning - wdyt?

@dilyanpalauzov

Copy link
Copy Markdown
Contributor
  • Not all errors from XBase during validation/parsing can be handled as warnings, some are real errors.

These errors come from https://github.com/eclipse-xtext/xtext/blob/main/org.eclipse.xtext.xbase/src/org/eclipse/xtext/xbase/validation/XbaseValidator.java, which contains:

warning("Redundant case.", casePart, null, IssueCodes.REDUNDANT_CASE);
error("Void functions cannot return a value.", expr, null,ValidationMessageAcceptor.INSIGNIFICANT_INDEX, INVALID_RETURN);
error("There is no context to infer the closure's argument types from. Consider typing the arguments or put the closures into a typed context.", closure, null, INSIGNIFICANT_INDEX, TOO_LITTLE_TYPE_INFORMATION);

or https://github.com/eclipse-xtext/xtext/blob/main/org.eclipse.xtext.xbase/src/org/eclipse/xtext/xbase/validation/EarlyExitValidator.java:

error("Unreachable expression.", expression, null, IssueCodes.UNREACHABLE_CODE);

One approach is to create a class, which derives from XbaseValidator, which overwrites some methods, by replacing error( with warn( and at the end in the runtimemodule to write:

// contributed by org.eclipse.xtext.xtext.generator.validation.ValidatorFragment2
@SingletonBinding(eager=true)
public Class<? extends XbaseValidator> bindXbaseValidator() {
  return NewXbaseValidator.class;  
}

Another approach is to ask upstream to change these errors into warnings.

Xbase — which is used by DSL Rules/Scripts/Transformations — has two modes of evaluation: Interpeted mode, which openHAB uses, and .java-files generation mode. Both modes use the same input. If

g.members.forEach[ u | logError(“A”, u.name + “|” + u.state + " | " + u.class.toString) ]

produces in interpeter mode the message

There is no context to infer the closure’s argument types from. Consider typing the arguments or put the closures into a typed context.

and after parsing it is interpreted at runtime correctly, in the other mode, when generating .java files, there is no execution mode: everything has to be determined at the time of parsing and generating .java files. (I guess!)

So if some input produces at parsing data, which cannot be converted immediately to .java files, Xbase fires an error. Irrespective of whether in interpreter mode this would be fine.

Or, third variant, find out how to turn all validation/parsing errors into warnings only for the Rules/Scripts/Transformations model. But this might not work, e.g. these errors:

error("Assignment to final variable", expression, feature, ValidationMessageAcceptor.INSIGNIFICANT_INDEX, ASSIGNMENT_TO_FINAL);
error("The maximum number of parameters for a closure is six.", closure, Literals.XCLOSURE__DECLARED_FORMAL_PARAMETERS, 6, TOO_MANY_PARAMS_IN_CLOSURE);

are errors and not warnings.

@dilyanpalauzov

Copy link
Copy Markdown
Contributor

If code is not going to change, then this could be added as “Breaking Change” under - https://github.com/openhab/openhab-distro/releases/tag/5.1.0#breaking-changes-that-require-manual-interaction-after-the-upgrade - XBase errors are not anymore converted to warnings.

@lolodomo

lolodomo commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

Do you happen to remember why we had to be strict?

Yes, there was a very good reason even if I don't yet remember exactly which one. It was to not load a wrong item.
The fix is just to restore the old behaviour for rules but keep the current code for others. It is just the test to change.

Edit: we can check where error is used in validation checks to remember the reason.

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@jimtng

jimtng commented Feb 14, 2026

Copy link
Copy Markdown
Contributor Author

@lolodomo
This was part of our discussion on errors vs warning #4928 (comment)

I've just pushed a commit that restores the errors/warnings but makes an exception for rules and scripts.

If we want to make rules/scripts stricter again in the future, we should do so in a separate PR.

}
if (!newWarnings.isEmpty()) {
logger.info("Validation issues found in DSL model '{}', using it anyway:\n{}", name,
logger.warn("Validation issues found in DSL model '{}', using it anyway:\n{}", name,

@lolodomo lolodomo Feb 14, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please keep the original log level.
We have a different log level for errors leading to model not being loaded and model being loaded but with warnings/tips.
With your change, everything will be a WARNING. And that would made the approach different from what was also done for YAML files.

Comment on lines +338 to +339
if (d.getSeverity() == org.eclipse.emf.common.util.Diagnostic.ERROR
&& !"rules".equals(modelType) && !"script".equals(modelType)) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is the fix I have in mind.

For consistency with the other places in that file, I would rather suggest:

!"rules".equalsIgnoreCase(resource.getURI().fileExtension()) && 
!"script".equalsIgnoreCase(resource.getURI().fileExtension())

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@lolodomo

lolodomo commented Feb 14, 2026

Copy link
Copy Markdown
Contributor

It looks good to me now. Thank you Jim.
Should also be backported to 5.1.x branch.

@lolodomo

Copy link
Copy Markdown
Contributor

Maybe you could add "for rules and scripts" in the PR title.

@jimtng jimtng changed the title Restore model validation not to fail on diagnostic errors Restore model validation not to fail on diagnostic errors for rules and scripts Feb 14, 2026
@jimtng

jimtng commented Feb 14, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for your help @lolodomo!

@dilyanpalauzov thanks for the background in xbase / xtend. Do you think going forward we should enforce stricter validations for rulesdsl or should we just leave it as is?

@kaikreuzer kaikreuzer left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@kaikreuzer
kaikreuzer merged commit f9776bc into openhab:main Feb 14, 2026
5 checks passed
@kaikreuzer kaikreuzer added this to the 5.2 milestone Feb 14, 2026
@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of the Core label Feb 14, 2026
@jimtng
jimtng deleted the model-error-handling branch February 14, 2026 12:32
kaikreuzer pushed a commit that referenced this pull request Feb 14, 2026
…nd scripts (#5351)

* Restore model validation not to fail on diagnostic errors

Signed-off-by: Jimmy Tanagra <jcode@tanagra.id.au>
@kaikreuzer kaikreuzer added the backported A PR that has been cherry-picked to a patch release branch label Feb 14, 2026
@kaikreuzer

Copy link
Copy Markdown
Member

Cherry-picked it to 5.1.x.

@dilyanpalauzov

Copy link
Copy Markdown
Contributor

Do you think going forward we should enforce stricter validations for rules dsl or should we just leave it as is?

I am not sure. As it was and is also now, more relaxed checking (errors are warnings) just works most of the time. But sometimes it prevents errors to pop-up during parsing, and I do not know what happens, if these errors happen at run-time.

@openhab-bot

Copy link
Copy Markdown
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/rules-and-rule-templates-yaml-integration/168568/194

@openhab-bot

Copy link
Copy Markdown
Collaborator

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/dsl-error-when-comparing-two-string-variables/169756/15

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backported A PR that has been cherry-picked to a patch release branch bug An unexpected problem or unintended behavior of the Core

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants