analysis server rename should not refactor files in the pub cache

[devoncarew]

Related to dart-atom/dart#339. The rename refactoring can return results from code in the pub cache. These can be valid references to the symbol in question, but we should not be refactoring package: code, or code in the pub cache.

[bwilkerson]

Ignoring references in packages would lead to broken code, so the right answer is probably to report a problem with the refactoring.

[scheglov]

I don't agree that we should not be refactoring package: code.
It may be a self import or a reference to another project, which is also in a project root.

The question is what to do with these external references.

1. Include or not include edits for these external files into returned change?
   I guess we don't want to include edits for external files, because users don't want to apply them and damage some external code.
2. Provide the list of external files in a long unstructured error message, or add some support for structured information?
   I guess nobody actually cares about this structured list, so we could ignore this for now.

[skybrian]

In the particular case I saw, it was a clear false positive. I renamed a method in my project. I had no intention of modifying package:test. The authors of package:test know nothing about my project, and the method wasn't in a unit test. It's production code that doesn't import anything in package:test at all.

I find this a rather alarming bug, particularly since it's not the first time it happened to me (see [1]) and nobody seems to be in much of a hurry to fix it. I'm able to adjust (and have already) but our tools are corrupting user data in a way that's likely to convince naive developers (and perhaps even experienced developers) that Dart dev tools are dangerous to use. When data corruption happens in production, server-side developers usually consider this very important to fix.

We should fix the atom plugin, the analyzer, and pub to prevent this from happening again, even if some other level screws it up. This is what server-side developers do when something bad goes wrong - you don't fix it in one place, you fix it at multiple places in the chain of failure.

I think I agree on the structured errors thing. At the IDE level, perhaps we can borrow an idea from IDEA? When refactoring Java, there's a distinction made between references found in code versus references found in comments and strings (which are less reliable). Similarly, in a Dart IDE when types aren't declared and we're just guessing based on the name matching, then perhaps the reference should go in a separate section of low-confidence matches.

At the analyzer level, it seems like we should be using the library import graph to remove false positives? If there's no chain of imports from library A to library B then a usage in A is unlikely to refer to a definition in B. Of course it might happen if you're not declaring any types and accessing fields dynamically, but it seems better to assume it's a false positive. Since most users aren't planning on modifying any packages they downloaded from pub and circular dependencies are rare, for most users filtering based on the import graph is likely to do the right thing.

At the pub level, permissions on files in .pub_cache should be set to read-only.

Or perhaps there are other ways to detect and prevent corruption that might be more effective?

[1] dart-lang/test#328

[devoncarew]

@scheglov, to clarify, by package code I was referring to code in the pub_cache, not other things referenced by package: references, like self-references.

@skybrian, re: the case you saw, if that particular reference was a 'potential' reference - identified as a possible but not certain reference by the analysis server - we're now filtering those out in Atom. They won't be shown to the user in the list of modified files, or acted on when applying edits. In a future iteration we may choose to keep the modification but present it to the user with a different style, give the user the ability to toggle edits in and out of the refactoring, and have the potential edits be unselected by default.

[bwilkerson]

Perhaps we need a third category of edits (similar to our 'potential' edits) for edits within external files (where by external I mean anything in either the pub cache or the SDK).

[scheglov]

Actually DAS already provides enough information for clients.
See the SourceFileEdit.file field, which is an absolute path to a file.
Using this field clients can decide whether they want to change the file or not.

For example, clients know their analysis roots (and sometimes DAS does not know about all of them, because the client may send only a subset of roots to the server). So, the client can check whether the file path is prefixed by one of the analysis roots, and apply the change or report a problem to the user.

Adding any additional flags for SourceFileEdit does not add anything useful to solve this problem.

[devoncarew]

But Konstantin, every client will need to put a check in to make sure that none of the edits are trying to modify the pub_cache. Modifying the pub_cache is not correct or desired behavior. It would be better if the fix was applied at the analysis server level, once, rather than every client needing to put this fix in.

[alexander-doroshko]

I'd agree with Devon. Pub cache and Dart SDK are files that never should be modified.
As for the other files/packages that are not within Analysis Server roots - I agree with Konstantin - client could decide itself.

[bwilkerson]

So, what would you like to see if a refactoring finds potential edits in the pub cache or SDK? Should we silently drop those edits? Should we drop them but include a non-fatal error? Should we include them but mark them?

What about the case where the edits are not potential (such as a user asking to rename "String")? Should we produce a fatal error? Should we include the edits?

Note, however, that the client still has to look at the file path of every file containing changes in order to decide whether to apply the edit (for example, if the file is marked as read only) and how to interact with the user in response to finding a problem. Because of this, it sounds to me like you're asking us to add complexity to the API for no value to the client. (But given that I'm not writing the client, perhaps I'm wrong.)

[alexander-doroshko]

My thoughts:

• if edits in SDK/Pub Cache are not potential -> do not suggest refactoring at all (I think it means returning an error in Analysis Server terms)
• if edits in SDK/Pub Cache are potential -> do not include these edits in refactoring (unlikely such edits can ever be useful).

AFAIK this doesn't require API change.

[devoncarew]

I'd be happy w/ the above.

[scheglov]

Don't allow renaming elements declared in SDK.
https://codereview.chromium.org/1377623002

[clayberg]

Konstantin - can you also do the same for elements in Pub Cache?

[scheglov]

Don't allow renaming in pub cache.
https://codereview.chromium.org/1380253003

[scheglov]

Done in c06ad0d