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

error details are misaligned when using tabs #1129

Closed
srenatus opened this issue Dec 21, 2018 · 2 comments · Fixed by #1138
Closed

error details are misaligned when using tabs #1129

srenatus opened this issue Dec 21, 2018 · 2 comments · Fixed by #1138
Labels

Comments

@srenatus
Copy link
Contributor

Actual Behavior

With demo.rego as

package demo

foo {
	as
}

where the line with as starts with a tab character, and I run opa run demo.rego, I find:

error: 1 error occurred during loading: demo.rego:5: rego_parse_error: no match found
                as
          ^

Expected Behavior

The pointer ^ should respect the tabs or remove them -- right now, it's position is calculated without accounting for the width of the characters that make up the offset here.

@tsandall tsandall added the bug label Jan 1, 2019
@tsandall
Copy link
Member

tsandall commented Jan 3, 2019

Thanks for filing this. I noticed it a while ago but failed to open a ticket 😅

In this case, the error is reported by the parser. The ^ is generated here: https://github.com/open-policy-agent/opa/blob/master/ast/parser_ext.go#L752 (not in check.go).

I was suspicious of the location you linked to but upon reflection, I don't think there's a problem. The padding is constructed based on the String representation of the ref -- which will not contain tabs.

I haven't thought about the best way to resolve the issue in parser_ext.go. Perhaps we could construct the padding with a mix of spaces & tabs?

@srenatus
Copy link
Contributor Author

srenatus commented Jan 4, 2019

I was suspicious of the location you linked to but upon reflection, I don't think there's a problem. The padding is constructed based on the String representation of the ref -- which will not contain tabs.

Yup, indeed, I was pointing at the wrong thing; I've too noticed that when I was attempting to fix this 😄

I think the real issue is somewhere in here, as there's nothing accounting for the fact that a char could be "wider" than 1 char when printed...

Maybe this could be fixed bluntly by a pre-processing step that replaces tabs by n spaces (for some n) before feeding it into the parser? 🤔

srenatus added a commit to srenatus/opa that referenced this issue Jan 8, 2019
Fixes open-policy-agent#1129 for the most part.

What still is wrongly put, but also harder to fix, is the case where
there's tabs INSIDE the line:

	p = TAB true TAB { TAB as }

or a space before the tab, like

	SPACE TAB p = true { as }

will still have a misaligned "^", as in that case, not all tabs are
fully expanded.

This merely trims leading tabs, and fixes the error maker location if
there's no other tabs used in the line.

However, that should fit common usage: Leading tabs is what `opa fmt`
proposes; and I'm doubtful of too many uses of tabs in other places
in Rego code.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
tsandall pushed a commit that referenced this issue Jan 8, 2019
Fixes #1129 for the most part.

What still is wrongly put, but also harder to fix, is the case where
there's tabs INSIDE the line:

	p = TAB true TAB { TAB as }

or a space before the tab, like

	SPACE TAB p = true { as }

will still have a misaligned "^", as in that case, not all tabs are
fully expanded.

This merely trims leading tabs, and fixes the error maker location if
there's no other tabs used in the line.

However, that should fit common usage: Leading tabs is what `opa fmt`
proposes; and I'm doubtful of too many uses of tabs in other places
in Rego code.

Signed-off-by: Stephan Renatus <srenatus@chef.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants