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

encoding/json: error positions do not reflect position of error #2776

Closed
rogpeppe opened this issue Jan 22, 2024 · 3 comments
Closed

encoding/json: error positions do not reflect position of error #2776

rogpeppe opened this issue Jan 22, 2024 · 3 comments
Assignees
Labels

Comments

@rogpeppe
Copy link
Member

rogpeppe commented Jan 22, 2024

What version of CUE are you using (cue version)?

$ cue version
cue version v0.7.0

Does this issue reproduce with the latest stable release?

Yes

What did you do?

exec go mod tidy
# The syntax error is on line 4, so we expect
# error message to include that.
exec go run main.go
stderr 'test\.json:4'
-- go.mod --
module m

go 1.21

require cuelang.org/go v0.7.0
-- test.json --
{
	"x": 2
}
	{"unterminated",}
-- main.go --
package main

import (
	"bytes"
	_ "embed"
	"fmt"
	"log"
	"os"

	"cuelang.org/go/cue/errors"
	"cuelang.org/go/encoding/json"
)

//go:embed test.json
var testJSON []byte

func main() {
	dec := json.NewDecoder(nil, "test.json", bytes.NewReader(testJSON))
	// Sanity check that the first value can be extracted OK.
	_, err := dec.Extract()
	if err != nil {
		log.Fatalf("first: %v", err)
	}
	_, err = dec.Extract()
	fmt.Printf("error: %v\n", err)
	fmt.Printf("positions: %v\n", errors.Positions(err))
	fmt.Println("errors.Print: ")
	errors.Print(os.Stdout, err, nil)
}

What did you expect to see?

A passing test

What did you see instead?

The position of the error bears no relationship to the position that the syntax error actually occurred at.
It always says it's at line 1, column 1.

# The syntax error is on line 4, so we expect
# error message to include that. (0.258s)
> exec go run main.go
[stdout]
error: invalid JSON for file "test.json": invalid character ',' after object key
positions: [test.json:1:1]
errors.Print: 
invalid JSON for file "test.json": invalid character ',' after object key:
    test.json:1:1
> stderr test,json:4
FAIL: /tmp/testscript3302001597/jsonpos.txtar/script.txtar:5: no match for `test,json:4` found in stderr
@rogpeppe rogpeppe added NeedsInvestigation Triage Requires triage/attention NeedsFix and removed NeedsInvestigation Triage Requires triage/attention labels Jan 22, 2024
cueckoo pushed a commit that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
cueckoo pushed a commit that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
cueckoo pushed a commit to cue-lang/cue-trybot that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](cue-lang/cue#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
Dispatch-Trailer: {"type":"trybot","CL":1175779,"patchset":3,"ref":"refs/changes/79/1175779/3","targetBranch":"master"}
cueckoo pushed a commit that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
cueckoo pushed a commit to cue-lang/cue-trybot that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](cue-lang/cue#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
Dispatch-Trailer: {"type":"trybot","CL":1175779,"patchset":4,"ref":"refs/changes/79/1175779/4","targetBranch":"master"}
cueckoo pushed a commit that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
cueckoo pushed a commit to cue-lang/cue-trybot that referenced this issue Jan 22, 2024
The base offset seems to be a hangover from the origins of the
package in [`go/token`](https://pkg.go.dev/go/token#FileSet.AddFile).
In that package, it signifies the overall offset inside a set of
line information shared amongst between many files.  In `cue/token`,
by contrast, it doesn't signify anything, witnessed by the fact
that when we _do_ try to use it, it [doesn't have the desired
effect](cue-lang/cue#2776).

In effect, the base is added to the internal index stored in a
`token.Pos` and then removed before it's made visible to the API.

Because it's confusing, and because in general we can't
say that we're starting at an arbitrary point in a file
and hope line and column information will be able to work,
we deprecate the API, maintaining all user-visible behavior.

Signed-off-by: Roger Peppe <[email protected]>
Change-Id: I57d0f54722bf4aa872ef1f36e2d2a8e2e43a5810
Dispatch-Trailer: {"type":"trybot","CL":1175779,"patchset":5,"ref":"refs/changes/79/1175779/5","targetBranch":"master"}
@mvdan
Copy link
Member

mvdan commented Jan 23, 2024

Is test,json:4 in the testscript a typo?

@rogpeppe
Copy link
Member Author

Is test,json:4 in the testscript a typo?

Yup, fixed.

@mvdan
Copy link
Member

mvdan commented Aug 8, 2024

Note that you created a duplicate of this issue in #3317, now fixed, but this reproducer above shows that we're not fully fixed for NDJSON or JSONL inputs. I'll send a follow-up fix using this tracking issue.

@mvdan mvdan self-assigned this Aug 8, 2024
cueckoo pushed a commit that referenced this issue Aug 12, 2024
For #2776.

Signed-off-by: Daniel Martí <[email protected]>
Change-Id: Id353bef957aa768a8a846f7cd0ef51ad9091d5b6
Reviewed-on: https://review.gerrithub.io/c/cue-lang/cue/+/1199102
TryBot-Result: CUEcueckoo <[email protected]>
Unity-Result: CUE porcuepine <[email protected]>
Reviewed-by: Roger Peppe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants