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

evalv3: disable CUE_DEBUG=openinline once the new evaluator is reasonably finished #3688

Open
mvdan opened this issue Jan 20, 2025 · 3 comments

Comments

@mvdan
Copy link
Member

mvdan commented Jan 20, 2025

Currently, CUE_EXPERIMENT=evalv3 is not enabled by default yet (#3662), but CUE_DEBUG=openinline is enabled by default, which affects evalv3 exclusively:

$ cue help environment
[...]
		openinline (default true)
			Permit disallowed fields to be selected into literal struct
			that would normally result in a close error, mimicking evalv2
			closedness behavior in evalv3 to aid the transition.

This basically emulates slightly broken closedness behavior from evalv2 in evalv3, which is fine in the near term for the sake of users transitioning to evalv3, but in the medium to long term, this debug flag should be disabled by default and eventually removed entirely.

This issue tracks the disabling of its default. For example, one full release after evalv3 has been enabled by default, if we are reasonably confident that most users are already on evalv3 and we have a plan for what the users relying on broken closedness can do.

@mvdan mvdan added Triage Requires triage/attention and removed Triage Requires triage/attention labels Jan 20, 2025
@myitcv
Copy link
Member

myitcv commented Jan 20, 2025

Noting as well something else we suggested, that we could enabled CUE_DEBUG=openinline as a function of the language version. For example, assuming we flip the default of evalv3 to "on" in v0.13, we could decide to only enable CUE_DEBUG=openinline=1 when the module language version is < v0.13.

@myitcv
Copy link
Member

myitcv commented Feb 1, 2025

I've just spent a significant amount of time tracking down what at first appeared to be a bug, but was in fact an artefact of the fact that openinline being enabled by default. Calmly noting that this isn't the first time I've been bitten by this!

Thinking out loud so that that thinking can be challenged.

The debug option of CUE_DEBUG=openline is a function of the experiment evalv3 being enabled, i.e. openline has no effect if CUE_EXPERIMENT=evalv3=0, when evalv2 is the evaluator.

As of 5991735, users get evalv2 unless they set CUE_EXPERIMENT=evalv3 or CUE_EXPERIMENT=evalv3=1. That is, we have evalv2 enabled by default. For users in this situation, by definition, openinline doesn't affect them, save for the fact that they are relying on the behaviour of evalv2.

This leaves:

  1. Existing CUE users with existing CUE modules manually migrating to evalv3 by setting CUE_EXPERIMENT=evalv3 or CUE_EXPERIMENT=evalv3=1.
  2. New CUE users or people creating new CUE modules experimenting with evalv3 by setting CUE_EXPERIMENT=evalv3 or CUE_EXPERIMENT=evalv3=1

openline was established to help people in group 1. The choosing of the default of "on" was based on the fact that this is a commonly relied-on behaviour of evalv2, and we didn't want the flip to evalv3 presenting as a "sharp edge" for people, deterring them from trying it. And if people in group 1 weren't relying on this behaviour in evalv2, then the default setting of openinline would in reality be a no-op.

This leaves people in group 2. For this group, with a default of openinline to on I think we risk:

  • Creating more work for ourselves down the line by people writing code against evalv3 that relies on openinline (without them knowing it)
  • Worsening the experience with evalv3 for people who spot or fall foul of the evalv2 "behaviour" (read: bug) that openinline papers over, i.e. where they spot that closedness is not "working properly". This might cause people to be less inclined to persevere with evalv3

With the default of openinline=1 we are also in a situation that, by default, creating a new module with 5991735, I have to explicitly set CUE_DEBUG=openinline=0 for correct behaviour of CUE. Whilst the merits of setting openinline=1 for group 1 were clear and well motivated, I'm not sure it's the right default on the whole.

As such, I'm not convinced that our current default of openline=1 for evalv3 is correct.

In #3688 (comment), I suggested that we could base the default on language.version, again for the sake of group 1. But this also has a very rough edge of different behaviour being observed between otherwise identical-looking code in two different modules.

Instead I think we should flip to default openinline=0. This would require people in group 1 to explicitly set CUE_DEBUG=openinline=1, but with a guide on "FAQs on migrating existing code to evalv3" (a page that should probably already exist) we can make that advice targeted. Indeed such a guide is very likely the thing we want to be pushing people in group 1 towards in the first place, rather than just asking them to blindly set CUE_EXPERIMENT=evalv3=1. It increases awareness of the problem/setting, and the conditions under which setting that CUE_DEBUG option might be required. And it leaves anyone needing to set the option with the clear impression that this is something they ultimately need to fix.

@mvdan
Copy link
Member Author

mvdan commented Feb 4, 2025

I can see arguments either way; shipping an upcoming release with the defaults evalv3=1,openinline=1 aims to make the upgrade from evalv2 as painless as possible for existing users by avoiding any regressions, even if they are due to closedness fixes. The original goal of evalv3 was to avoid regressions at nearly any cost, especially if they affected many users, and in this case we found a handful of users who were relying on the old closedness behavior.

But, as you point out, for new users, the defaults evalv3=1,openinline=0 make a lot more sense. Plus, they prevent new code from falling into the same mistake of relying on broken closedness.

So I'm personally starting to lean towards openinline=0 as a default. Whichever choice we make, it will be painful for some users, and I'd lean towards getting the correct behavior out of the box.

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

No branches or pull requests

2 participants