if-else with '=' assignment, wrong curly placement #259

lefec · 2017-10-25T11:41:27Z

Hi, I didn't find a previous issue about this.

I have this (working) code:

x = 5

if(x >= 5)
  y = TRUE else 
    y = FALSE

With style_active_region() producing:

if (x >= 5) {
  y
} <- TRUE else
  y <- FALSE

It breaks my code here.

The text was updated successfully, but these errors were encountered:

lorenzwalthert · 2017-10-25T12:48:04Z

Thanks @lefec for reaching out. This is indeed very unfortunate. There are two aspects to it.

If you replace the assignment = with <-, you won't get invalid code back. The problem here is really an inconsistency in the R parser discussed in inconsistency in getParseData? #100.
The indention of the braces will still not be correct, i.e.

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-10-25

library(styler)
style_text("x = 5
if(x >= 5)
y <- TRUE else
  y <- FALSE
")
#> x <- 5
#> if (x >= 5) {
#>   y <- TRUE
#>   } else {
#>   y <- FALSE
#>   }

Which is an issue already reported #257 and which I hope to solve soon.

@krlmlr What do you suggest? Should we just disable adding braces if there is any = assignment in the code in order to prevent (1) from happening? Invalidating code is the worst styler can do anyways and I think it should be avoided at any cost. Eventually we need to fix the parsing inconsistency but I think that might take some time.

krlmlr · 2017-10-25T13:06:30Z

I understand that even if #100 is closed the underlying problems are not resolved completely:

styler:::create_tree("if (TRUE) a = 1")
#> Warning: replacing previous import 'scales::viridis_pal' by
#> 'viridis::viridis_pal' when loading 'DiagrammeR'
#>                                              levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {id})
#> 2   °--expr:  [0/0] {16}                              
#> 3       ¦--IF: if [0/1] {1}                           
#> 4       ¦--'(': ( [0/0] {2}                           
#> 5       ¦--expr:  [0/0] {4}                           
#> 6       ¦   °--NUM_CONST: TRUE [0/0] {3}              
#> 7       ¦--')': ) [0/1] {5}                           
#> 8       ¦--expr:  [0/1] {9}                           
#> 9       ¦   °--SYMBOL: a [0/0] {7}                    
#> 10      ¦--EQ_ASSIGN: = [0/1] {8}                     
#> 11      °--expr:  [0/0] {11}                          
#> 12          °--NUM_CONST: 1 [0/0] {10}

What does it take to put the EX_ASSIGN and the expr/NUM_CONST where it belongs? Can we compute the parent information ourselves from line + col data, to avoid relying on the (sometimes) faulty parent ID in the parse data?

lorenzwalthert · 2017-10-25T22:35:18Z

I haven't looked into the solution Jim Hester provided in #100, but when mentioning #100, I just meant to "solve" the parsing problem, i.e. getting a consistent parse data (that is, the same parsing data as if <- was used instead of =, just with text being different for the assignment token) which would solve the problem. That involves getting the right parent from the parse data. I think this is not trivial. We can also parse, replace the tokens EQ_ASSIGN with arrow assignment, serialise, parse again and replace text from the second serialisation with the text from the first, which is not particularly elegant.

krlmlr · 2017-10-26T07:50:26Z

Compare the parse tree between arrow assignment and equals assignment:

styler:::create_tree("a <- b <- 1")
#>                                              levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {id})
#> 2   °--expr:  [0/0] {11}                              
#> 3       ¦--expr:  [0/1] {3}                           
#> 4       ¦   °--SYMBOL: a [0/0] {1}                    
#> 5       ¦--LEFT_ASSIGN: <- [0/1] {2}                  
#> 6       ¦--expr:  [0/1] {6}                           
#> 7       ¦   °--SYMBOL: b [0/0] {4}                    
#> 8       ¦--LEFT_ASSIGN: <- [0/1] {5}                  
#> 9       °--expr:  [0/0] {8}                           
#> 10          °--NUM_CONST: 1 [0/0] {7}
styler:::create_tree("a = b = 1")
#>                                             levelName
#> 1 ROOT (token: short_text [lag_newlines/spaces] {id})
#> 2  ¦--expr:  [0/1] {3}                               
#> 3  ¦   °--SYMBOL: a [0/0] {1}                        
#> 4  ¦--EQ_ASSIGN: = [0/1] {2}                         
#> 5  ¦--expr:  [0/1] {6}                               
#> 6  ¦   °--SYMBOL: b [0/0] {4}                        
#> 7  ¦--EQ_ASSIGN: = [0/1] {5}                         
#> 8  °--expr:  [0/0] {8}                               
#> 9      °--NUM_CONST: 1 [0/0] {7}

Seems that all we need to do here is to wrap EQ_ASSIGN sequences into a separate expression. Maybe this operator is special because it has lowest precedence?

Walk all nests
Look at sequences of EQ_ASSIGN
If there is one in the nest, wrap everything between the token before the first EQ_ASSIGN and the token after the last EQ_ASSIGN into a separate expression, very similarly to the addition of braces (Wrap expr in expr before enclosing with curly braces #263).

lorenzwalthert · 2017-10-26T18:33:51Z

Ok, I see. Looks like that could be a valid approach. Need to look at more examples maybe. We could try to fix it in the nested structure or the flat structure. If we fix it in the flat structure (that is, reassigning parents and ids before nesting), this might have the advantage that we fix the problem early in the food chain, so other packages / tasks that do nesting in a different way than we do might benefit. However, that might be potentially more difficult.

lorenzwalthert · 2017-11-02T08:32:16Z

Ok, I guess for the time being I just want to follow your suggestion and fix the problem in the nested parse data.

lorenzwalthert · 2017-11-02T09:32:49Z

I think you can't do it exactly that way because in the case below, two = appear on the same level but there is an else in between that you don't want to wrap.

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-11-02

styler:::create_tree("if (TRUE) \na<- 3 else b <- 4")
#>                                                  levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {pos_id})
#> 2   °--expr:  [0/0] {1}                                   
#> 3       ¦--IF: if [0/1] {2}                               
#> 4       ¦--'(': ( [0/0] {3}                               
#> 5       ¦--expr:  [0/0] {5}                               
#> 6       ¦   °--NUM_CONST: TRUE [0/0] {4}                  
#> 7       ¦--')': ) [0/0] {6}                               
#> 8       ¦--expr:  [1/1] {7}                               
#> 9       ¦   ¦--expr:  [0/0] {9}                           
#> 10      ¦   ¦   °--SYMBOL: a [0/0] {8}                    
#> 11      ¦   ¦--LEFT_ASSIGN: <- [0/1] {10}                 
#> 12      ¦   °--expr:  [0/0] {12}                          
#> 13      ¦       °--NUM_CONST: 3 [0/0] {11}                
#> 14      ¦--ELSE: else [0/1] {13}                          
#> 15      °--expr:  [0/0] {14}                              
#> 16          ¦--expr:  [0/1] {16}                          
#> 17          ¦   °--SYMBOL: b [0/0] {15}                   
#> 18          ¦--LEFT_ASSIGN: <- [0/1] {17}                 
#> 19          °--expr:  [0/0] {19}                          
#> 20              °--NUM_CONST: 4 [0/0] {18}
styler:::create_tree("if (TRUE) \na = 3 else b = 4")
#>                                                  levelName
#> 1  ROOT (token: short_text [lag_newlines/spaces] {pos_id})
#> 2   °--expr:  [0/0] {1}                                   
#> 3       ¦--IF: if [0/1] {2}                               
#> 4       ¦--'(': ( [0/0] {3}                               
#> 5       ¦--expr:  [0/0] {5}                               
#> 6       ¦   °--NUM_CONST: TRUE [0/0] {4}                  
#> 7       ¦--')': ) [0/0] {6}                               
#> 8       ¦--expr:  [1/1] {8}                               
#> 9       ¦   °--SYMBOL: a [0/0] {7}                        
#> 10      ¦--EQ_ASSIGN: = [0/1] {9}                         
#> 11      ¦--expr:  [0/1] {11}                              
#> 12      ¦   °--NUM_CONST: 3 [0/0] {10}                    
#> 13      ¦--ELSE: else [0/1] {12}                          
#> 14      ¦--expr:  [0/1] {14}                              
#> 15      ¦   °--SYMBOL: b [0/0] {13}                       
#> 16      ¦--EQ_ASSIGN: = [0/1] {15}                        
#> 17      °--expr:  [0/0] {17}                              
#> 18          °--NUM_CONST: 4 [0/0] {16}

krlmlr · 2017-11-02T09:50:25Z

Good point. I guess we need to look for sequences of EQ_ASSIGN that are interspersed with exactly one other token.

lorenzwalthert · 2017-11-02T09:53:17Z

Yes, I agree. So I am just trying to first split a nest into blocks, each containing one valid = expression and then map over the blocks like this.

relocate_eq_assign_nest <- function(pd) {
  is_eq_assign <- which(pd$token == "EQ_ASSIGN")
  if (length(is_eq_assign) > 0) {
    browser()
    block_id <-
      cumsum((is_eq_assign - lag(is_eq_assign, default = is_eq_assign[1])) > 2)
    blocks <- split(pd, block_id)
    pd <- map_dfr(blocks, relocate_eq_assign_one)
  }
  pd
}

Well not exactly, but somehow like this.

krlmlr · 2017-11-02T09:58:36Z

Can you use diff()?

lorenzwalthert · 2017-11-02T11:29:01Z

I modified create_treee() so it can also return just the structure of an expression, i.e

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-11-02

styler:::create_tree(
  "x <- 5
  
  if(x >= 5)
  y <- TRUE else 
  y <- FALSE",
  structure_only = TRUE
)
#>                 levelName
#> 1  Hierarchical structure
#> 2   ¦--1                 
#> 3   ¦   ¦--1             
#> 4   ¦   ¦   °--1         
#> 5   ¦   ¦--2             
#> 6   ¦   °--3             
#> 7   ¦       °--1         
#> 8   °--2                 
#> 9       ¦--1             
#> 10      ¦--2             
#> 11      ¦--3             
#> 12      ¦   ¦--1         
#> 13      ¦   ¦   °--1     
#> 14      ¦   ¦--2         
#> 15      ¦   °--3         
#> 16      ¦       °--1     
#> 17      ¦--4             
#> 18      ¦--5             
#> 19      ¦   ¦--1         
#> 20      ¦   ¦   °--1     
#> 21      ¦   ¦--2         
#> 22      ¦   °--3         
#> 23      ¦       °--1     
#> 24      ¦--6             
#> 25      °--7             
#> 26          ¦--1         
#> 27          ¦   °--1     
#> 28          ¦--2         
#> 29          °--3         
#> 30              °--1

I think I managed to solve the problem, so we have

reprex::reprex_info()
#> Created by the reprex package v0.1.1.9000 on 2017-11-02

all.equal(
  styler:::create_tree("a <- b <- 1", structure_only = TRUE),
  styler:::create_tree("a =  b = 1", structure_only = TRUE)
)
#> [1] TRUE

all.equal(
  styler:::create_tree(
  "x = 5
  
  if(x >= 5)
    y = TRUE else 
      y = FALSE",
  structure_only = TRUE
  ),
  styler:::create_tree(
    "x <- 5
    
    if(x >= 5)
    y <- TRUE else 
    y <- FALSE",
    structure_only = TRUE
  )
)
#> [1] TRUE

Will push later.

lorenzwalthert · 2017-11-08T19:43:10Z

@lefec finally hoping to close this with #276.

lorenzwalthert added Priority: Critical Status: WIP and removed Status: WIP labels Oct 25, 2017

This was referenced Oct 26, 2017

Consider removing column id and parent from parse table #265

Closed

Adding roundtrip for nested processing #140

Closed

lorenzwalthert added this to the CRAN milestone Oct 30, 2017

lorenzwalthert mentioned this issue Nov 2, 2017

Refactoring #270

Merged

lorenzwalthert added Complexity: Medium Status: WIP Type: Bug labels Nov 2, 2017

lorenzwalthert mentioned this issue Nov 8, 2017

Fix eq assign parsing #276

Merged

lorenzwalthert closed this as completed in #276 Nov 10, 2017

lorenzwalthert removed the Status: WIP label Nov 19, 2017

lorenzwalthert mentioned this issue Mar 14, 2018

Substring duplication for dplyr::mutate #373

Closed

lorenzwalthert mentioned this issue Mar 1, 2019

return() should be on its own line #473

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

if-else with '=' assignment, wrong curly placement #259

if-else with '=' assignment, wrong curly placement #259

lefec commented Oct 25, 2017

lorenzwalthert commented Oct 25, 2017

Uh oh!

krlmlr commented Oct 25, 2017

Uh oh!

lorenzwalthert commented Oct 25, 2017

Uh oh!

krlmlr commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017

Uh oh!

krlmlr commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017 •

edited

Loading

Uh oh!

krlmlr commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017 •

edited

Loading

Uh oh!

lorenzwalthert commented Nov 8, 2017

Uh oh!

if-else with '=' assignment, wrong curly placement #259

if-else with '=' assignment, wrong curly placement #259

Comments

lefec commented Oct 25, 2017

lorenzwalthert commented Oct 25, 2017

Uh oh!

krlmlr commented Oct 25, 2017

Uh oh!

lorenzwalthert commented Oct 25, 2017

Uh oh!

krlmlr commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Oct 26, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017

Uh oh!

krlmlr commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

krlmlr commented Nov 2, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lorenzwalthert commented Nov 8, 2017

Uh oh!

lorenzwalthert commented Nov 2, 2017 •

edited

Loading

lorenzwalthert commented Nov 2, 2017 •

edited

Loading