Remove WEXPERIMENTAL for "if with unsupported statements" by mandolaerik · Pull Request #332 · intel/device-modeling-language

mandolaerik · 2024-10-23T12:20:34Z

Remove WEXPERIMENTAL for "if with unsupported statements"

syssimics · 2024-10-23T13:08:30Z

Verification #14013436: ✅ pass

lwaern-intel

Looks pretty good.

lwaern-intel · 2024-10-23T14:34:37Z

py/dml/messages.py

+    kind of statement as long as the `#if` condition doesn't reference
+    any identifiers other than `dml_1_2`, `true`, and `false`.
+    """
+    fmt = "object of type %s not allowed inside `#if`"


%s declarations are not allowed inside #if

Or maybe %s statements are not ...? Note that this is a best-effort error message, will e.g. say "extern_typedef statements are ..." which is not optimal but good enough. The important ones are param, import and template, and we get those right at least.

How come you prefer statements instead of declarations? Sure, we have a concept of "object statements", but as you point out, many of the forbidden things doesn't fit inside that concept; and we have "declaration" as a collective name for object statements and global declarations (which sometimes is not ideal; for example in our documentation we often say "param declarations" as a collective name for all param object statements, which I dislike, but that's what we do. If we say "param declarations not allowed inside #if" I don't think people would get confused.)

Also, this error message should be tagged version = "1.4". Toplevel if logic can only happen in 1.4 code.

I just realized. The emission of this error message disagrees with non-toplevel ifs. They crop up from invocations of validate_if_body() from dmlparse.py; and those use ECONDP, ECONDT and ECONDINEACH. We'll have to address that; replace them all with EBADCONDSTMT?

I don't see a contradiction here. The messages for non-top-level ifs do not need to mention that top-level ifs have a special case. They are also more specific than top-level ifs, and I don't see a problem with that. I would probably not oppose unifying the messages, but that can be done in a separate PR.

The messages for non-top-level ifs do not need to mention that top-level ifs have a special case.

Well, no, I'm not arguing that (we've settled that a time ago.) I'm confused why you're bringing that up; what are you addressing with that statement? Or are you talking about documentation -- "the documentation for error messages of non-top-level ifs do not need to mention that top-level ifs have a special case, thus it's a good thing they're separate error classes?" I don't buy that argument at all.

They are also more specific than top-level ifs, and I don't see a problem with that.

Neither do I. What I have a problem with is that your PR introduces an inconsistency about what error classes are used for reporting the same kind of error (from the user's perspective.) Because of that, I don't buy the argument that unification can be done in a separate PR.
Thus I'm arguing either you replace ECONDP, ECONDT, and ECONDINEACH all with EBADCONDSTMT, or you replace EBADCONDSTMT with uses of ECONDP, ECONDT, and ECONDINEACH. Either way, you can implement it by having ast builder pass what forbidden declarations are in use, and then you can either report ECONDP/ECONDT/ECONDINEACH for each of them; or you can pass the sites and declaration kinds to EBADCONDSTMT to each be printed via print_site_message.

What I have a problem with is that your PR introduces an inconsistency

It's not an inconsistency toward the end user. The same kind of mistake will always give an error, and in some circumstances it will use a somewhat different wording (essentially because there's more error cases to cover). The end result is that the user will read the message and adjust their code, and then they will not see the message again. What's the problem? The error tags are mainly related to our internal testing.

Hm, what we could do is to pick a more directed message, like "not allowed inside if, except dml_1_2" if the violating statement is one that only can appear on top level, like a typedef or template. And use a plain ECONDP etc for the other cases. Not sure if that makes anyone happier, though.

Thus I'm arguing either you replace ECONDP, ECONDT, and ECONDINEACH all with EBADCONDSTMT, or you replace EBADCONDSTMT with uses of ECONDP, ECONDT, and ECONDINEACH.

I still insist on this. It makes no sense to me to have different error classes be used depending on if the #if is on top-level or not. And it does matter to the end user since we talk about errors in terms of error tags both in documentation and in conversation with our users (speaking of which we still need to enforce -T to be always on. I don't remember why we've procrastinated that).

Separately, you haven't addressed the original problem I raised of the fmt formulation being bad

Honestly I like the idea of getting rid of ECONDP and ECONDT in favor of EBADCONDSTMT. In any case, just to reiterate, you do not need to tailor the error message to be different for top-level #ifs. If you want, you could tailor the error message for top-level #ifs to say "not allowed inside if, except dml_1_2" or something similar like you suggest, but that's entirely up to you; though I'll note that an accurate formulation would prove difficult, especially since we're gonna want to allow the use of _breaking_change params in the conditionals of special top-level #ifs.

doc/1.4/language.md

lwaern-intel · 2024-10-23T14:38:39Z

RELEASENOTES.md

  various operations that rely on the `register_view` interface, such as the
  `dev_util.bank_regs` function and the `write-device-reg` and `probe-address`
  CLI commands.
+- `note 6` DMLC no longer emits warnings saying


will no longer emit the subset of WEXPERIMENTAL warnings saying...

maybe?

I chose not to because I didn't want to bore the reader with details, but it does make sense to mention WEXPERIMENTAL because of how --no-warn=WEXPERIMENTAL is sprinkled in makefiles.

Yeah that's mostly why I figured it was a good idea.

still not mention of WEXPERIMENTAL, here. I still think you should do so, but whichever way you go about it is up to you.

lwaern-intel · 2024-10-23T14:39:34Z

RELEASENOTES.md

+  `param` and `template` are forbidden inside `#if`, but a special exception
+  allows forbidden statements to appear specifically inside an `#if (dml_1_2)`
+  block. The warning message was meant to highlight this irregularity, but
+  caused more harm than good.


more harm than good; error messages surrounding the special case have been improved instead.

lwaern-intel · 2024-11-19T12:54:44Z

RELEASENOTES.md

  various operations that rely on the `register_view` interface, such as the
  `dev_util.bank_regs` function and the `write-device-reg` and `probe-address`
  CLI commands.
+- `note 6` DMLC no longer emits warnings saying


Toplevel if logic is 1.4 exclusive, and thus so should this releasenote.

I disagree. You are right that they are exclusive for 1.4 files, but their only use is in the context of 1.2 devices: The only allowed identifier is dml_1_2, so the construct is only relevant for common code intended for use in 1.2.

so the construct is only relevant for common code intended for use in 1.2.

Only for the time being. I think we've both agreed upon extending the conditional to also allow the use of the compat params?

Nevertheless, I guess this is a matter of perspective of where a releasenote belongs. I was thinking "1.4, because this feature is only available in 1.4 files", while you're arguing the use of it is only significant due to how it impact 1.2 devices. Because of that, I'm fine with it being in RELEASENOTES.dml; but I would note that your argument sounds like it'd belong in RELEASENOTES-1.2.md, which I would heavily disagree with.

The releasenote split between 1.2/1.4/both is inherently fuzzy and somewhat questionable. What matters the most is that the end-user sees a note attached to the right package version, this happens no matter what releasenote file we pick; the choice of file is not so important.

I guess the advantage of the split is that many syntax additions are 1.4 specific, and putting it in -1.4 eliminates the need to mention this in text.

I think it would make sense at least to merge RELEASENOTES and RELEASENOTES-1.2. A total 7 notes were 1.2 specific over the past 3 years.

So what action are we taking?

syssimics · 2024-11-20T12:06:02Z

Verification #14117325: ✅ pass

syssimics · 2026-03-09T13:21:00Z

PR Verification: ✅ success

lwaern-intel · 2026-03-09T13:22:46Z

py/dml/messages.py

+    kind of statement as long as the `#if` condition doesn't reference
+    any identifiers other than `dml_1_2`, `true`, and `false`.
+    """
+    fmt = "object of type %s not allowed inside `#if`"


Thus I'm arguing either you replace ECONDP, ECONDT, and ECONDINEACH all with EBADCONDSTMT, or you replace EBADCONDSTMT with uses of ECONDP, ECONDT, and ECONDINEACH.

I still insist on this. It makes no sense to me to have different error classes be used depending on if the #if is on top-level or not. And it does matter to the end user since we talk about errors in terms of error tags both in documentation and in conversation with our users (speaking of which we still need to enforce -T to be always on. I don't remember why we've procrastinated that).

Separately, you haven't addressed the original problem I raised of the fmt formulation being bad

lwaern-intel · 2026-03-09T13:24:02Z

RELEASENOTES.md

  various operations that rely on the `register_view` interface, such as the
  `dev_util.bank_regs` function and the `write-device-reg` and `probe-address`
  CLI commands.
+- `note 6` DMLC no longer emits warnings saying


So what action are we taking?

lwaern-intel · 2026-03-09T13:28:16Z

RELEASENOTES.md

  various operations that rely on the `register_view` interface, such as the
  `dev_util.bank_regs` function and the `write-device-reg` and `probe-address`
  CLI commands.
+- `note 6` DMLC no longer emits warnings saying


still not mention of WEXPERIMENTAL, here. I still think you should do so, but whichever way you go about it is up to you.

lwaern-intel · 2026-03-09T13:41:33Z

py/dml/messages.py

+
+    A special exception is that a `#if` on top scope may contain any
+    kind of statement as long as the `#if` condition doesn't reference
+    any identifiers other than `dml_1_2`, `true`, and `false`.


Don't forget dml_1_4!

wait. actually you can't reference dml_1_4. what? Did I trick myself into believing that? That's pretty bad since I've communicated that you're able to reference dml_1_4 to everyone I've explained experimental #if to...

That feels like a bad inconsistency. I mean, I got confused about it. Maybe you should add dml_1_4 to be able to be referenced. I ask that totally not at all out of a self-interested need to retroactively make my improper communication proper, I promise

py/dml/messages.py

lwaern-intel · 2026-03-09T13:44:05Z

py/dml/messages.py

+
+    A special exception is that a `#if` on top scope may contain any
+    kind of statement as long as the `#if` condition doesn't reference
+    any identifiers other than `dml_1_2`, `true`, and `false`.


wait. actually you can't reference dml_1_4. what? Did I trick myself into believing that? That's pretty bad since I've communicated that you're able to reference dml_1_4 to everyone I've explained experimental #if to...

That feels like a bad inconsistency. I mean, I got confused about it. Maybe you should add dml_1_4 to be able to be referenced. I ask that totally not at all out of a self-interested need to retroactively make my improper communication proper, I promise

lwaern-intel · 2026-03-09T13:47:59Z

doc/1.4/language.md


+As a special exception, an `#if` statement that appears on top level is allowed
+to contain any type of statement, as long as the condition doesn't reference
+any identifiers other than `dml_1_2`, `true` and `false`.


If you do add dml_1_4 you'll need to update this accordingly.

The reason for "only 1.2" is that I want to encourage users to consistently use the dml_1_2 form, for two reasons (or both are really the same reason):

Currently dml_1_2 is equivalent to !dml_1_4, but in a world where 1.2/1.4/1.6 coexist they are not, and in this case dml_1_2 better expresses what you mean: you want stuff to change when you switch between "old" and "new". Comparing for equality to 1.2 is future proof because we will never add new old versions.

Saying 1.2 in code is a clear marker that the #if is a legacy thing that can safely be removed once the codebase is free from DML 1.2 code. If we say 1.4, then this is less obvious.

For this reason, I don't yet see a real use case where dml_1_4 is preferable over dml_1_2, so I don't see a compelling reason to spend time here.

MMmmfmffhghh... fine.

actually, no -- not fine. I don't understand why your favored approach is to use the restrictions placed on top-level #ifs as a vehicle against dml_1_4. If you dislike dml_1_4 so much and want to discourage its use -- why does it exist at all in the first place? I'd prefer we remove dml_1_4 entirely rather than having this strange inconsistency.

syssimics · 2026-03-09T20:00:37Z

PR Verification: ❌ failure

mandolaerik requested a review from lwaern-intel October 23, 2024 12:33

lwaern-intel requested changes Oct 23, 2024

View reviewed changes

lwaern-intel requested changes Nov 19, 2024

View reviewed changes

mandolaerik force-pushed the pr/remove-wexperimental-for-if-with-unsupported-statements branch from e5f4b1b to cd00065 Compare November 20, 2024 11:18

mandolaerik requested review from KingoftheHomeless and lwaern-intel November 20, 2024 11:18

mandolaerik force-pushed the pr/remove-wexperimental-for-if-with-unsupported-statements branch 2 times, most recently from 3a45f08 to 222d6aa Compare March 9, 2026 12:26

syssimics removed the request for review from KingoftheHomeless March 9, 2026 12:26

mandolaerik requested a review from KingoftheHomeless March 9, 2026 12:27

lwaern-intel requested changes Mar 9, 2026

View reviewed changes

lwaern-intel reviewed Mar 9, 2026

View reviewed changes

lwaern-intel requested changes Mar 9, 2026

View reviewed changes

py/dml/messages.py Outdated Show resolved Hide resolved

lwaern-intel reviewed Mar 9, 2026

View reviewed changes

mandolaerik added 2 commits March 9, 2026 20:34

Remove WEXPERIMENTAL for "if with unsupported statements"

c714bf8

Merge ECONDP and ECONDT into EBADCONDSTMT

496bac9

mandolaerik force-pushed the pr/remove-wexperimental-for-if-with-unsupported-statements branch from 222d6aa to 496bac9 Compare March 9, 2026 19:34

syssimics requested review from lwaern-intel and removed request for KingoftheHomeless March 9, 2026 19:35

Conversation

mandolaerik commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

syssimics commented Oct 23, 2024

Uh oh!

lwaern-intel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KingoftheHomeless Oct 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Mar 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lwaern-intel Nov 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

syssimics commented Nov 20, 2024

Uh oh!

syssimics commented Mar 9, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

mandolaerik commented Oct 23, 2024 •

edited

Loading

KingoftheHomeless Oct 26, 2024 •

edited

Loading

lwaern-intel Nov 21, 2024 •

edited

Loading

lwaern-intel Mar 9, 2026 •

edited

Loading

lwaern-intel Nov 21, 2024 •

edited

Loading