On save, lintr raises "unexpected '/'" error

[otherdoug]

Issue

When saving R script files in Atom, lintr called via languageserver output reports an error unexpected '/' in line 1 regardless of script content.

Otherwise lintr output generally works as expected when making inline edits. Further typing without saving runs lintr again, and produces accurate results:

Potential cause

The unexpected '/' error appears to come from running lintr::lint on a file URI that looks like the text of an R script instead of a path to an R script:

> # File URI string with newline at the end
> lintr::lint('file:///Users/myusername/local/path/to/the/file/deleteme.R\n', linters = NULL)
<text>:1:6: error: unexpected '/'
file:///Users/myusername/local/path/to/the/file/deleteme.R

> # File path
> lintr::lint('/Users/myusername/local/path/to/the/file/deleteme.R', linters = NULL)
/Users/myusername/local/path/to/the/file/deleteme.R:5:1: style: Lines should not be more than 80 characters.
tmp <- "This string is so long that it should raise a linter info message, but it won't."
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> 

The did-save / text-sync interface appears to be origin. The interface stores the file URI in params$textDocument and stores what I assume is the document content in params$text though I have not been able to devise an example that actually stores anything there.

languageserver/R/interfaces.R

Lines 378 to 386 in 61259eb

	did_save_text_document_params <- function(uri, text) {
	structure(
	list(
	textDocument = uri,
	text = text
	),
	class = "did_save_text_document_params"
	)
	}

When text_document_did_save(...) is called, params$text partial matches with params$textDocument when params$text is undefined. This causes content to populate from stringi::stri_split_lines(text)[[1]] by splitting the file URI as text, rather than loading the file URI from the path stringi::stri_read_lines(path).

languageserver/R/handlers-textsync.R

Lines 63 to 85 in 61259eb

	text_document_did_save <- function(self, params) {
	textDocument <- params$textDocument
	text <- params$text
	uri <- uri_escape_unicode(textDocument$uri)
	logger$info("did save:", list(uri = uri))
	path <- path_from_uri(uri)
	if (!is.null(text)) {
	content <- stringi::stri_split_lines(text)[[1]]
	} else if (file.exists(path)) {
	content <- stringi::stri_read_lines(path)
	} else {
	content <- NULL
	}
	if (self$workspace$documents$has(uri)) {
	doc <- self$workspace$documents$get(uri)
	doc$set_content(doc$version, content)
	} else {
	doc <- Document$new(uri, NULL, content)
	self$workspace$documents$set(uri, doc)
	}
	self$text_sync(uri, document = doc, run_lintr = TRUE, parse = TRUE)
	}

When lintr::lint is eventually called on content in diagnose_file(...) we get the unexpected '/' error because a file URI like file:///blah... isn't valid R syntax, and the unexpected forward-slash is where that's detected.

languageserver/R/diagnostics.R

Lines 95 to 97 in 61259eb

	text <- paste0(content, collapse = "\n")
	diagnostics <- lapply(
	lintr::lint(text, linters = linters), diagnostic_from_lint, content = content)

Attempted fixes

I've made a hacky substitution of textContent for text in the two blocks above to avoid partial-matching text with textDocument and it appears to resolve the issue. However, I'll be the first to admit I'm not familiar with the languageserver interface, so I don't know what else may be expecting param$text to exist.

did_save_text_document_params <- function(uri, text) { 
   structure( 
     list( 
       textDocument   = uri, 
       textContent    = text 
     ), 
     class = "did_save_text_document_params" 
   ) 
 } 

text_document_did_save <- function(self, params) { 
     textDocument <- params$textDocument 
     text <- params$textContent 
     uri <- uri_escape_unicode(textDocument$uri) 
     logger$info("did save:", list(uri = uri)) 

   . . . 
}

Looking at the surrounding code in textsync.R, I also notice that text_document_did_open uses text <- textDocument$text to populate text, rather than text <- params$text. Perhaps someone more familiar with the spec may be able to say whether this difference is intentional or accidental.

languageserver/R/handlers-textsync.R

Lines 5 to 10 in 61259eb

	text_document_did_open <- function(self, params) {
	textDocument <- params$textDocument
	uri <- uri_escape_unicode(textDocument$uri)
	version <- textDocument$version
	text <- textDocument$text
	logger$info("did open:", list(uri = uri, version = version))

Environment details

I am using Atom 1.48.0 with packages:

• atom-ide-ui
• atom-language-r
• ide-r

Potentially related issues

#250 -- looks like the same lintr error output

[randy3k]

Could you report the debug log from Atom?

Open the devtools console, run atom.config.set('core.debugLSP', true) and restarting atom.
You should then able to see the debug log on the console. (run atom.config.set('core.debugLSP', false) to disable it)

[otherdoug]

Here you go: atom_debug.log

That's opening Atom, saving the file, modifying the file, saving the file again.

Please let me know if I've missed anything, or if anything else would be helpful.

[renkun-ken]

A simple fix is to use textDocument[["text"]] instead of textDocument$text to avoid partial matching in this case, or we should make sure both element exist in the list in the first place.

[renkun-ken]

Maybe we should go through all these interfaces and see if there are potential risks of partial matching in other places.

[randy3k]

How about switching to https://rdrr.io/cran/xfun/man/strict_list.html? Though I don't know if there will be some overheads.

[randy3k]

I guess textDocument[["text"]] should be an easier solution. @otherdoug Thank you for diving into the cause.

[renkun-ken]

Somewhere in the R documentation suggests that $ is only recommeded for interactive use and [[x]] for programmatic use where there's no partial matching.

[renkun-ken]

I go through all the interfaces and did_save_text_document_params is the only one partial match could happen.

[renkun-ken]

@otherdoug Are you interested in putting up a PR? New contributors are always welcome!

[otherdoug]

Sure—I should be able put something together this weekend.