Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
8898001:
Convert "library unit" File path into "absolute unit path", issue 590. (Closed)
|
Created:
9 years ago by scheglov Modified:
8 years, 11 months ago CC:
reviews_dartlang.org Visibility:
Public. |
DescriptionConvert "library unit" File path into "absolute unit path", issue 590.
http://code.google.com/p/dart/issues/detail?id=590
Actually URI is not expectedto be absolute path to the unit, this is just ID.
And Compiler provides it in form pathToTheLibraryUnit/unitName.dart
So, I've tweaked converting URI into File.
R=brianwilkerson@google.com
BUG=
TEST=
Committed: https://code.google.com/p/dart/source/detail?r=3002
Patch Set 1 #Patch Set 2 : Fix URI for diet source #Messages
Total messages: 15 (0 generated)
I think you're attacking the problem in the wrong place. In my mind the real question is why Source.getUri() would ever return a URI with a path of the form "/home/scheglov/dart/Test/Test.dart/Foo.dart"? This is not a valid path (in the sense that there is no such file). I think we should find the places where such a URI is created and change them to create a valid URI (and look to understand whether there are any dependencies on these invalid URI's that also need to be fixed). (Not to mention the fact that this will break in the admittedly unlikely event that a user creates a directory named "Test.dart".)
On 2011/12/09 16:03:17, Brian Wilkerson wrote: > I think you're attacking the problem in the wrong place. In my mind the real > question is why Source.getUri() would ever return a URI with a path of the form > "/home/scheglov/dart/Test/Test.dart/Foo.dart"? This is not a valid path (in the > sense that there is no such file). I think we should find the places where such > a URI is created and change them to create a valid URI (and look to understand > whether there are any dependencies on these invalid URI's that also need to be > fixed). I just talked to Joel and the reason for the Library+Unit is to keep the output artifacts segregated from each other (libA and libB refer to A.dart). This problem was actually brought up by the guys in SEA a long time ago.
> I just talked to Joel and the reason for the Library+Unit is to keep the output > artifacts segregated from each other (libA and libB refer to A.dart). This > problem was actually brought up by the guys in SEA a long time ago. That's both very clever, and very ugly. There should be a method like getUniqueArtifactId() that returns a munged path that can be used for this purpose, and getUri() should return the absolute URI of the source. Any chance we can fix the API rather than putting in a work-around?
I just ran a little experiment. I loaded frog into the Editor and printed out every URL that was associated with the source associated with an error event. The list will be in the next message because of a 10000 character/message limit. There are a some interesting observations related to this list: 1. Only the last 20 are invalid URI's that could not be used as is. All of the rest can trivially be resolved to files so that the editor can associate the errors with the right source file. 2. There are 51 other errors that have a valid URI that are associated with the same file as the 20 that are invalid. 3. The valid URI's include both files that define a library and files that do not. My conclusion is that the URI mangling is not being consistently applied. From what I've seen I have not been able to determine any pattern for when the mangling is applied. I initially thought that perhaps it was only being applied when it was proven that it was needed, but the fact that the same file has two paths associated with it seems to preclude that conclusion. The inconsistent mangling might be causing other problems than just making it hard to associate error messages with files. For example, if the same file can have two paths associated with it, then it appears that it can have two artifacts associated with it. When the final output is generated, which artifacts are being used, and do they have the right content?
Here's the list of the URI's that were printed: dart://html/src/MessageEvent.dart dart://html/src/MessageEvent.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/elements/elements.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/typechecker.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/elements/elements.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/tree/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/tree/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/tree/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/scanner/listener.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/scanner/parser.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/scanner/array_based_scanner.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/compiler.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/compiler.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/namer.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/namer.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/resolver.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/tree_validator.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/tree_validator.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/typechecker.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/universe.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/builder.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/nodes.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/tracer.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/leg/ssa/tracer.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/file_system_node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart file:/Users/brianwilkerson/src/dart-public/dart/frog/lib/node/node.dart/node.dart
On 2011/12/09 17:59:21, Brian Wilkerson wrote: > Here's the list of the URI's that were printed: Is this code path, getResource(File), only getting getting called from the error handler? We could added to Source a getFileUri() to return the actual file on disk.
> Is this code path, getResource(File), only getting getting called from the error > handler? We could added to Source a getFileUri() to return the actual file on > disk. No, it's used in several other code paths as well. But there are other places on the path from the error handler where we could use a method like that to fix the problem. (I would make the change in CompilerListener.getResource(DartCompilationError), and would be happy to make the editor-side changes once the API is available.) Personally I think it would be a better API design to use getUri() for a valid URI and something else (getUniqueArtifactId(), for example) for something that returns a thing that isn't necessarily a valid URI. I think there will be less confusion long term. But the important point is that we need two methods because we're using them for two different purposes and they return two different values. In either case, someone needs to look at all uses of Source.getUri() to determine whether they should be using the mangled form to identify an artifact or the valid form to identify a file.
Adding Joel for reference. I don't think that's a bad idea. On 2011/12/09 18:19:06, Brian Wilkerson wrote: > > Is this code path, getResource(File), only getting getting called from the > error > > handler? We could added to Source a getFileUri() to return the actual file on > > disk. > > No, it's used in several other code paths as well. But there are other places on > the path from the error handler where we could use a method like that to fix the > problem. (I would make the change in > CompilerListener.getResource(DartCompilationError), and would be happy to make > the editor-side changes once the API is available.) > > Personally I think it would be a better API design to use getUri() for a valid > URI and something else (getUniqueArtifactId(), for example) for something that > returns a thing that isn't necessarily a valid URI. I think there will be less > confusion long term. But the important point is that we need two methods because > we're using them for two different purposes and they return two different > values. > > In either case, someone needs to look at all uses of Source.getUri() to > determine whether they should be using the mangled form to identify an artifact > or the valid form to identify a file.
I've uploaded new patch which fixes URI for diet unit from LibraryUnit. With it Editor does not show anymore errors from other units in library unit. Basically, we now return same URI as was in original (non-diet) unit. However there is still problem. When you have errors in library unit and other unit, and you change/save one of them, then Problems view shows only errors in saved unit, problems from other unit are gone. As I can see, you provide this other unit from CachingArtifactProvider, with API/diet version of unit. And this version is parsed and replaces errors from full-source version of unit.
DBC I talked to scheglov offline about this and figured out follow up in case it was not clear to anyone else. It sounds like the mangled URI only appears on diet parsed units since it was a deserialization issue. So, UrlSource.getUri should never return a mangled name and UrlDartSource.getName will always return mangled names. This patch addresses the serialization issue. How does the editor determine when to clear the error markers? Does it do it for all units in the program or only for those units (non-diet) which where actually compiled? On 2012/01/04 20:19:42, scheglov wrote: > I've uploaded new patch which fixes URI for diet unit from LibraryUnit. > With it Editor does not show anymore errors from other units in library unit. > Basically, we now return same URI as was in original (non-diet) unit. > > However there is still problem. > When you have errors in library unit and other unit, and you change/save one of > them, then Problems view shows only errors in saved unit, problems from other > unit are gone. As I can see, you provide this other unit from > CachingArtifactProvider, with API/diet version of unit. And this version is > parsed and replaces errors from full-source version of unit.
On 2012/01/04 20:56:29, mmendez wrote: > DBC > > I talked to scheglov offline about this and figured out follow up in case it was > not clear to anyone else. It sounds like the mangled URI only appears on diet > parsed units since it was a deserialization issue. So, UrlSource.getUri should > never return a mangled name and UrlDartSource.getName will always return mangled > names. This patch addresses the serialization issue. > > How does the editor determine when to clear the error markers? Does it do it > for all units in the program or only for those units (non-diet) which where > actually compiled? As I can see com.google.dart.tools.core.internal.builder.DartcBuildHandler.buildAllApplications(IProject, boolean, IProgressMonitor) always clears all markers and then asks DartCompiler to compile library. It seems however that all units are compiled (may be not, because when I saw this, I've had compilation problems in other units), but other units are in diet form.
> > How does the editor determine when to clear the error markers? Does it do it > > for all units in the program or only for those units (non-diet) which where > > actually compiled? > > As I can see ... always clears all markers and then asks DartCompiler > to compile library. That is correct. The code that you're referring to was written before dartc was incremental, so before each compile we removed all of the markers for the library and then everything was re-compiled. That is no longer the case. (I didn't realize until today that we had this bug, and I will start work on it first thing in the morning.) There are three approaches that I can think of. First, we could never return any artifacts from the provider, essentially forcing dartc to re-compile everything. I think that would be too slow, so I don't like it. Second, we could add an "about to compile unit" notification to DartCompilerListener that would give us a chance to clear out the markers for the compilation unit that was about to be compiled. That would be clean, but would require some API changes. Third, we could use the existing DartCompilerListener.unitCompiled notification to do effectively the same thing. If we cache compilation errors that are reported during the compilation of a unit until the unit is done being compiled, then we could remove the old markers and create the new markers after the fact. That's a fairly small code change, and will be the first thing I explore just to see how it goes. Of course, both approaches assume that we're only getting compilation errors associated with the compilation unit's that are actually being re-compiled. In other words, compiling A.dart shouldn't create errors associated with B.dart. I expect that that's a valid assumption, but would appreciate being told if that's not the case.
On 2012/01/04 22:48:33, Brian Wilkerson wrote: > > > How does the editor determine when to clear the error markers? Does it do > it > > > for all units in the program or only for those units (non-diet) which where > > > actually compiled? > > > > As I can see ... always clears all markers and then asks DartCompiler > > to compile library. > > That is correct. The code that you're referring to was written before dartc was > incremental, so before each compile we removed all of the markers for the > library and then everything was re-compiled. That is no longer the case. (I > didn't realize until today that we had this bug, and I will start work on it > first thing in the morning.) > > There are three approaches that I can think of. First, we could never return any > artifacts from the provider, essentially forcing dartc to re-compile everything. > I think that would be too slow, so I don't like it. Agree. But may be we will have to do this. > Second, we could add an "about to compile unit" notification to > DartCompilerListener that would give us a chance to clear out the markers for > the compilation unit that was about to be compiled. That would be clean, but > would require some API changes. I like this approach. One thing to remember is that parsing Source into DartUnit can produce errors too, so this new notification should be done using Source. But if this was parsing of diet Source, we should not clear markers. May be we should pass "diet" flag or even include it into DartSource. There is already "diet" flag in DartUnit. > Third, we could use the existing DartCompilerListener.unitCompiled notification > to do effectively the same thing. If we cache compilation errors that are > reported during the compilation of a unit until the unit is done being compiled, > then we could remove the old markers and create the new markers after the fact. > That's a fairly small code change, and will be the first thing I explore just to > see how it goes. > > Of course, both approaches assume that we're only getting compilation errors > associated with the compilation unit's that are actually being re-compiled. In > other words, compiling A.dart shouldn't create errors associated with B.dart. I > expect that that's a valid assumption, but would appreciate being told if that's > not the case. I think actually that there are changes which cause new error or warnings in other units, when you change some unit. For example when you declare top-level method in A.dart, and B.dart has top-level method with same name, you should get 2 errors - 1 in A.dart and 1 in B.dart. Even if diet parsing will help you here, error in B.dart will be reported with wrong location in diet unit (wrong for original unit). And at second, declaring local variable with same name in B.dart, you should get warning. If you will not parse _full_ B.dart at same time, you will not get it. But may be we still can avoid parsing everything for every change. I don't know how JDT manages this, but it works pretty fast, may be Java is simpler in this respect. As I understand, only top-level declarations, like methods and classes are visible in other units. So, if unit can potentially "depend" on something external, and there are (or were to remove duplication warning) such externally-visible top-level entities in changed unit, then we should recompile such potential units. Here "depend" may mean "references something with name", i.e. we should collect names of all referenced or declared (for duplicate warnings) classes, methods, variables, etc for each parsed unit and save somewhere.
So, back to the CL, if I'm understanding correctly and the compiler will now report compilation errors with the unmodified (and correct) URI and the only remaining problem is the way the editor is (or is failing to) manage the errors that are being reported, then LGTM. We can sort out the editor's issues in a different set of CLs. (I got the impression somewhere along the way that the URI being returned might sometimes be the URI of the library file rather than the URI of the compilation unit containing the error, which would clearly not be acceptable, but that's probably an incorrect impression.) |
