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

Validate target type for all option fields, not just features #279

Merged
merged 2 commits into from
Apr 16, 2024

Conversation

jhump
Copy link
Member

@jhump jhump commented Apr 12, 2024

The code to verify that a feature field was only used on particular kind of element was features-specific. However, in protoc, it is not features-specific and applies to all option fields that have a targets option defined.

This PR is a bit complicated because I had to change the error handling of the *interpreter.fieldValue method (and the setOptionField method that called it, as well as numerous methods that it used). It previously either returned a value or an error. If it returned an error, it would be reported by the caller. Even in the previous iteration of the code, this was not optimal because it meant that we had to abort processing an option value after the first error, instead of being able to potentially report multiple errors in the same option. But now, since the targets usage really doesn't impact the rest of the processing of an option value, it seems weird to abort that processing just because of a target type usage issue.

So now, the *interpreter.fieldValue method handles the errors that come up and only returns a non-nil error if the handler returns a non-nil error. That means calling code can no longer rely on err == nil but must separately check that the other returned value is valid/non-nil.

This change is the cause for most of the change/churn in the code.

The actual change to target type validation is a net simplification, which is nice.

@jhump jhump force-pushed the jh/validate-all-opt-field-target-types branch from d41ca4c to 60f81f4 Compare April 15, 2024 17:18
@jhump jhump changed the base branch from main to jh/linker-tests-checks-ALL-errors April 15, 2024 17:19
Base automatically changed from jh/linker-tests-checks-ALL-errors to main April 15, 2024 19:46
jhump added 2 commits April 15, 2024 16:30
…ential nil dereference panic that isn't covered by tests, so fixed all references to interp.resolver that weren't doing nil check
@jhump jhump force-pushed the jh/validate-all-opt-field-target-types branch from 60f81f4 to 5ecc9ad Compare April 15, 2024 20:31
@jhump jhump marked this pull request as ready for review April 15, 2024 23:25
@jhump jhump requested a review from emcfarlane April 15, 2024 23:25
@jhump
Copy link
Member Author

jhump commented Apr 15, 2024

@emcfarlane, this is the reason I did the pre-factoring in #281, so we have coverage of the way the code can now keep going and return both a nil/invalid result and a nil error.

There are changes to the message literal processing that involved un-indenting a block of code (to improve the control flow with an if (...) { ...; continue } ... instead of if (...) { ... } else { ... }). So I'd recommend selecting "hide whitespace" when viewing the diff.

options/options.go Show resolved Hide resolved
options/options.go Show resolved Hide resolved
@jhump jhump merged commit e8799f7 into main Apr 16, 2024
8 checks passed
@jhump jhump deleted the jh/validate-all-opt-field-target-types branch April 16, 2024 22:26
kralicky pushed a commit to kralicky/protocompile that referenced this pull request May 19, 2024
…ld#279)

The code to verify that a feature field was only used on particular kind
of element was features-specific. However, in `protoc`, it is not
features-specific and applies to _all_ option fields that have a
`targets` option defined. So this updates protocompile to behave the
same way.

This PR is a bit complicated because I had to change the error handling
of the `*interpreter.fieldValue` method (and the `setOptionField` method
that called it, as well as numerous methods that it used). It previously
either returned a value or an error. If it returned an error, it would
be reported by the caller. But now, these places handle the errors, to
allow reporting of multiple errors for the same option value. This change
in behavior is the reason for most of the churn/change in the code.

The change to just the target type validation is actually a net simplification.

(cherry picked from commit e8799f7)
kralicky pushed a commit to kralicky/protocompile that referenced this pull request Jun 8, 2024
…ld#279)

The code to verify that a feature field was only used on particular kind
of element was features-specific. However, in `protoc`, it is not
features-specific and applies to _all_ option fields that have a
`targets` option defined. So this updates protocompile to behave the
same way.

This PR is a bit complicated because I had to change the error handling
of the `*interpreter.fieldValue` method (and the `setOptionField` method
that called it, as well as numerous methods that it used). It previously
either returned a value or an error. If it returned an error, it would
be reported by the caller. But now, these places handle the errors, to
allow reporting of multiple errors for the same option value. This change
in behavior is the reason for most of the churn/change in the code.

The change to just the target type validation is actually a net simplification.

(cherry picked from commit e8799f7)
jhump added a commit that referenced this pull request Aug 20, 2024
)

The panic fix is tiny. But this commit is bigger because other
changes/improvements were called for: most of the code changes are to
improve error handling when in lenient mode. After I fixed the panic,
the test case was failing in a different way, due to an issue with how
errors (even in lenient mode) were still being passed to an error
reporter and causing the stage to fail.

This issue was introduced in #279. That PR moved things around, pushing
the responsibility of calling `interp.reporter.HandleError` down, so
that interpreting a single option could potentially report multiple
errors (instead of failing fast and only reporting the first).

But when in lenient mode, we don't actually want to send those errors to
the reporter: sending the error to the reporter meant the error would
get memoized and then returned in subsequent handle calls, which could
cause the process to fail when it should be lenient and also can cause
it to report the wrong error in lenient mode. So now we demarcate the
parts of the process where errors are tolerated (i.e. where lenience
mode is actually activated) with a new field on the interpreter that is
examined when errors are reported.
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

Successfully merging this pull request may close these issues.

2 participants