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

Not a spacing error: "<-chan <-chan" #655

Closed
fxkr opened this issue Dec 13, 2015 · 3 comments · Fixed by #678
Closed

Not a spacing error: "<-chan <-chan" #655

fxkr opened this issue Dec 13, 2015 · 3 comments · Fixed by #678

Comments

@fxkr
Copy link

fxkr commented Dec 13, 2015

package main

func main() {
    _ = make(<-chan <-chan struct{})
}

https://play.golang.org/p/CKUYB_PX5D

The space between the two "<-chan" is mistakenly shown as a spacing error.

@fatih
Copy link
Owner

fatih commented Jan 3, 2016

Hi @fxkr, seem it's caused by the rule for showing errors of chan <-, whereas it's expecting chan<-.

fatih added a commit that referenced this issue Jan 3, 2016
This is a very weird error. We want to show an error if the chan
statement is in form of:

	<- chan
	chan <-

The correct form should be of course:

	<-chan
	chan<-

But for example, if we have a channel of channels (like below), the
selected piece of whitespace is also highlighted as an error

	chan<- chan<-
	      ^

Because it's the part of `<- chan`, whereas we should not treat as an
error because we have two `chan<-` and `chan<-` statements.

To fix this, we add a `negative lookbehind` regex statement. It will not
match a text if it's preceded with other text.

First let assemble the current regex for the case: `<- chan`
(receive-only annotation), which is:

	\(<-\)\@<=\s\+\(chan\>\)\@=

The first part is:

	\(<-\)\@<=\s\+\

Here the pattern \@<= matches with zero width if the preceding atom
matches just before what follows. `\s\+` means white space of at least
one character. So it matches **whitespaces** which precedes the
statement `<-`

Now the second part is only useful with the first part. I've added the
placeholder to make it more readable:

	PLACEHOLDER(chan\>\)\@="

First the pattern `\@=` means to match the preceding atom with zero
width. So it will match the **PLACEHOLDER** till it finds a `chan`
statement. Once it finds it, it doesn't proceed anymore. So we only
highlight the whitespaces between `<-` and `chan` only if we have a
white space of at least one character

Finally we can introduce our fix now. We're going to pick the `<-`
statemetn if it doesn't follow a `chan`. So `chan<-` should NOT trigger
an error anymore, because we're not going to pick arrows in that form.

To match an arrow which doesn't follow a chan statement can be done via
a `negative lookbehind`, which is in the form of:

	\(chan\)\@<!<-

This literally means: "Match `<-` only if it doesn't follow `chan`.
We're going to use this pattern for the plain `<-` pattern. Not we're
just going to replace this into our current regex:

This original regex:

	\(<-\)\@<=\s\+\(chan\>\)\@=

will be changed into:

	\(\(chan\)\@<!<-\)\@<=\s\+\(chan\>\)\@=

Fixes #655
@fatih
Copy link
Owner

fatih commented Jan 3, 2016

Hi @fxkr, can you please pull #678 and let me know if it works for you?

@fxkr
Copy link
Author

fxkr commented Jan 3, 2016

Thanks!

Unfortunately, it introduces a regression. This correct code is now hilighted:

var mychan <-chan int

Luckily chan (the keyword) is always a word by itself (<- aren't word chars), so you can just always use \<chan\> instead of chan in all of these regexes and that fixes that, so:

" Spacing errors around the 'chan' keyword
if g:go_highlight_chan_whitespace_error != 0
  " receive-only annotation on chan type
  "
  " \(chan\)\@<!<-  (only pick arrow when it doesn't come after a chan)
  " this prevents picking up 'chan<- chan<-' but not '<- chan'
  syn match goSpaceError display "\(\(\<chan\>\)\@<!<-\)\@<=\s\+\(\<chan\>\)\@="

  " send-only annotation on chan type
  "
  " \(<-\)\@<!chan  (only pick chan when it doesn't come after an arrow)
  " this prevents picking up '<-chan <-chan' but not 'chan <-'
  syn match goSpaceError display "\(\(<-\)\@<!\<chan\>\)\@<=\s\+\(<-\)\@="

  " value-ignoring receives in a few contexts
  syn match goSpaceError display "\(\(^\|[={(,;]\)\s*<-\)\@<=\s\+f"
endif

(Interestingly, the second chan in the first regex already had the \>)

...and that fixes the problem and doesn't introduce any others as far as I can see.

Here's the test file I've been using:

package main

// Set  let g:go_fmt_autosave=0  in vimrc  before saving this file!

func main() {

    // ok = bad code highlighted and good code not highlighted
    // broken = bad code not highlighted or good code highlighted


    // Correctly formatted code
    // ------------------------

    _ = make(<-chan <-chan struct{}) // broken before, fixed

    // https://golang.org/ref/spec#Channel_types
    type _ struct {

        // "The <- operator associates with the leftmost chan possible:"
        _ chan<- chan int   // broken -> ok
        _ chan<- <-chan int // ok
        _ <-chan <-chan int // broken -> ok
        _ chan (<-chan int) // ok

        // "same as ..."
        _ chan<- (chan int)   // ok
        _ chan<- (<-chan int) // ok
        _ <-chan (<-chan int) // ok

        my1chan <-chan int // ok -> broken !!! -> ok with my fix
        my2chan chan<- int // ok -> ok (but probably by accident, because "chan chan<-" is ok)
    }


    // Code containing a single formatting error per line
    // --------------------------------------------------

    type _ struct {

        // Single bad space
        _ chan <- (chan int) // ok
        _ chan<- <- chan int // ok
        _ chan ( <-chan int) // broken

        // Multiple spaces within
        _ chan<-  chan int // ok
        _ chan  (<-chan int) // broken
        _ chan (<-chan  int) // broken
        _ chan<-  <-chan int // broken
    }
}

As you can see, there are some cases of incorrectly formatted code not being highlighted, but that was like that before your patch.

fatih added a commit that referenced this issue Jan 3, 2016
This is a very weird error. We want to show an error if the chan
statement is in form of:

	<- chan
	chan <-

The correct form should be of course:

	<-chan
	chan<-

But for example, if we have a channel of channels (like below), the
selected piece of whitespace is also highlighted as an error

	chan<- chan<-
	      ^

Because it's the part of `<- chan`, whereas we should not treat as an
error because we have two `chan<-` and `chan<-` statements.

To fix this, we add a `negative lookbehind` regex statement. It will not
match a text if it's preceded with other text.

First let assemble the current regex for the case: `<- chan`
(receive-only annotation), which is:

	\(<-\)\@<=\s\+\(chan\>\)\@=

The first part is:

	\(<-\)\@<=\s\+\

Here the pattern \@<= matches with zero width if the preceding atom
matches just before what follows. `\s\+` means white space of at least
one character. So it matches **whitespaces** which precedes the
statement `<-`

Now the second part is only useful with the first part. I've added the
placeholder to make it more readable:

	PLACEHOLDER(chan\>\)\@="

First the pattern `\@=` means to match the preceding atom with zero
width. So it will match the **PLACEHOLDER** till it finds a `chan`
statement. Once it finds it, it doesn't proceed anymore. So we only
highlight the whitespaces between `<-` and `chan` only if we have a
white space of at least one character

Finally we can introduce our fix now. We're going to pick the `<-`
statemetn if it doesn't follow a `chan`. So `chan<-` should NOT trigger
an error anymore, because we're not going to pick arrows in that form.

To match an arrow which doesn't follow a chan statement can be done via
a `negative lookbehind`, which is in the form of:

	\(chan\)\@<!<-

This literally means: "Match `<-` only if it doesn't follow `chan`.
We're going to use this pattern for the plain `<-` pattern. Not we're
just going to replace this into our current regex:

This original regex:

	\(<-\)\@<=\s\+\(chan\>\)\@=

will be changed into:

	\(\(chan\)\@<!<-\)\@<=\s\+\(chan\>\)\@=

Fixes #655
fatih added a commit that referenced this issue Jan 3, 2016
This is a very weird error. We want to show an error if the chan
statement is in form of:

	<- chan
	chan <-

The correct form should be of course:

	<-chan
	chan<-

But for example, if we have a channel of channels (like below), the
selected piece of whitespace is also highlighted as an error

	chan<- chan<-
	      ^

Because it's the part of `<- chan`, whereas we should not treat as an
error because we have two `chan<-` and `chan<-` statements.

To fix this, we add a `negative lookbehind` regex statement. It will not
match a text if it's preceded with other text.

First let assemble the current regex for the case: `<- chan`
(receive-only annotation), which is:

	\(<-\)\@<=\s\+\(chan\>\)\@=

The first part is:

	\(<-\)\@<=\s\+\

Here the pattern \@<= matches with zero width if the preceding atom
matches just before what follows. `\s\+` means white space of at least
one character. So it matches **whitespaces** which precedes the
statement `<-`

Now the second part is only useful with the first part. I've added the
placeholder to make it more readable:

	PLACEHOLDER(chan\>\)\@="

First the pattern `\@=` means to match the preceding atom with zero
width. So it will match the **PLACEHOLDER** till it finds a `chan`
statement. Once it finds it, it doesn't proceed anymore. So we only
highlight the whitespaces between `<-` and `chan` only if we have a
white space of at least one character

Finally we can introduce our fix now. We're going to pick the `<-`
statemetn if it doesn't follow a `chan`. So `chan<-` should NOT trigger
an error anymore, because we're not going to pick arrows in that form.

To match an arrow which doesn't follow a chan statement can be done via
a `negative lookbehind`, which is in the form of:

	\(\<chan\>\)\@<!<-

This literally means: "Match `<-` only if it doesn't follow `chan`.
We're going to use this pattern for the plain `<-` pattern. We also fix
the issue of only matching `chan` and not words like `mychan`. Aslo note
we're just going to replace this into our current regex:

This original regex:

	\(<-\)\@<=\s\+\(\<chan\>\)\@=

will be changed into:

	\(\(\<chan\>\)\@<!<-\)\@<=\s\+\(\<chan\>\)\@=

Fixes #655
@fatih fatih closed this as completed in #678 Jan 3, 2016
StabbyCutyou pushed a commit to StabbyCutyou/vim-go that referenced this issue Jan 4, 2016
This is a very weird error. We want to show an error if the chan
statement is in form of:

	<- chan
	chan <-

The correct form should be of course:

	<-chan
	chan<-

But for example, if we have a channel of channels (like below), the
selected piece of whitespace is also highlighted as an error

	chan<- chan<-
	      ^

Because it's the part of `<- chan`, whereas we should not treat as an
error because we have two `chan<-` and `chan<-` statements.

To fix this, we add a `negative lookbehind` regex statement. It will not
match a text if it's preceded with other text.

First let assemble the current regex for the case: `<- chan`
(receive-only annotation), which is:

	\(<-\)\@<=\s\+\(chan\>\)\@=

The first part is:

	\(<-\)\@<=\s\+\

Here the pattern \@<= matches with zero width if the preceding atom
matches just before what follows. `\s\+` means white space of at least
one character. So it matches **whitespaces** which precedes the
statement `<-`

Now the second part is only useful with the first part. I've added the
placeholder to make it more readable:

	PLACEHOLDER(chan\>\)\@="

First the pattern `\@=` means to match the preceding atom with zero
width. So it will match the **PLACEHOLDER** till it finds a `chan`
statement. Once it finds it, it doesn't proceed anymore. So we only
highlight the whitespaces between `<-` and `chan` only if we have a
white space of at least one character

Finally we can introduce our fix now. We're going to pick the `<-`
statemetn if it doesn't follow a `chan`. So `chan<-` should NOT trigger
an error anymore, because we're not going to pick arrows in that form.

To match an arrow which doesn't follow a chan statement can be done via
a `negative lookbehind`, which is in the form of:

	\(\<chan\>\)\@<!<-

This literally means: "Match `<-` only if it doesn't follow `chan`.
We're going to use this pattern for the plain `<-` pattern. We also fix
the issue of only matching `chan` and not words like `mychan`. Aslo note
we're just going to replace this into our current regex:

This original regex:

	\(<-\)\@<=\s\+\(\<chan\>\)\@=

will be changed into:

	\(\(\<chan\>\)\@<!<-\)\@<=\s\+\(\<chan\>\)\@=

Fixes fatih#655
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 a pull request may close this issue.

2 participants