431753 - Dehydra/Treehydra: better source locations on warning() and error()

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

I think this applies more to Treehydra, but I'm not sure.

The locations given by the builtins warning() and error() are usually not useful (and in fact distracting and confusing) in Treehydra. They seem to always give the last line of the file. It would be better to either (a) not give the location automatically, so it can be filled in by the user in the message, or (b) let the user specify the location in the call.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

Here's a patch for this. There could be some questions about the API. I set it up so that

  error("foo")

works the old way (just uses GCC current location), and

  error("foo", loc)

works the new way: loc should be a Treehydra location object (e.g., return of location_of). If not, it will just silently work the old way. (Possible problem: makes debugging harder. I left it off to avoid getting into reporting errors while reporting errors, but let me know what you think.) 

This does change the old API, which allowed any number of args, and printed an error for each one. That behavior didn't seem particularly important, and isn't used in any scripts I could find, so I thought it would be OK.

The other significant change is that I added a _source_location to Treehydra location objects. The GCC diagnostic functions seem to need the original source_location (aka location_t) value, which is an opaque int.

The final issue is that this doesn't do Dehydra any good, because Dehydra scripts never see GCC source_location values. Does Dehydra need this feature? If so, we could cover Dehydra by using Treehydra location objects in Dehydra.

Comment 2

[(dormant account)]

(In reply to comment #1)
> Created an attachment (id=318944) [details]
> Proposed patch
> 
> Here's a patch for this. There could be some questions about the API. I set it
> up so that
> 
>   error("foo")
> 
> works the old way (just uses GCC current location), and
> 
>   error("foo", loc)
> 
> works the new way: loc should be a Treehydra location object (e.g., return of
> location_of). If not, it will just silently work the old way. (Possible
> problem: makes debugging harder. I left it off to avoid getting into reporting
> errors while reporting errors, but let me know what you think.) 
> 
> This does change the old API, which allowed any number of args, and printed an
> error for each one. That behavior didn't seem particularly important, and isn't
> used in any scripts I could find, so I thought it would be OK.

Should just have a lowlevel _diagnostic function that doesn't have optional params and js warning/error functions that do the appropriate magic.


> 
> The other significant change is that I added a _source_location to Treehydra
> location objects. The GCC diagnostic functions seem to need the original
> source_location (aka location_t) value, which is an opaque int.
> 
> The final issue is that this doesn't do Dehydra any good, because Dehydra
> scripts never see GCC source_location values. Does Dehydra need this feature?
> If so, we could cover Dehydra by using Treehydra location objects in Dehydra.

Sounds like an excellent idea. a treehydra location + appropriate toString() func should be a drop-in solution.

r++ on the approach. I like how you got around not being able to pass custom locations to gcc.

Comment 3

[David Mandelin [:dmandelin]]

(In reply to comment #2)
> Should just have a lowlevel _diagnostic function that doesn't have optional
> params and js warning/error functions that do the appropriate magic.

I get the general concept but I'm not quite sure what you have in mind. Would you mind writing down your proposed API for the _diagnostic, warning, and error functions so that I can comment on it and/or code it up?

Comment 4

[(dormant account)]

I meant make a wrapper for return gcc_diagnostic(error, cx, obj, argc, argv, rval) to look like
gcc_diagnostic(isError(false for warning), msg: string, loc(undefined for letting gcc decide...but typically this should be function_decl.loc))

Then in pure js, we'd have warning/error functions that support current features. I'm not including the warning # feature that dehydra current has because I'm not sure it was ever more useful than a neat hack.

Comment 5

[Benjamin Smedberg]

Can you pass a string as the location instead of a treehydra tree object? I would like to be able to override the location from a dehydra script (see http://hg.mozilla.org/mozilla-central/index.cgi/file/f337857f6837/xpcom/analysis/static-checking.js#l218)

Comment 6

[(dormant account)]

Note in the example above you are passing a preexisting location to the error. In the new world order, it would be automagically be a treehydra location object. 
Just to clarify, the treehydra location object is a special case, it's not a lazy node, just a fully standalone object like those in dehydra.

Comment 7

[David Mandelin [:dmandelin]]

Passing a string looks difficult: the underlying GCC function can be controlled using the super-secret %H format-string command with a value of type source_location, but I didn't see anything for doing it with a string. 

Based on my browsing of GCC code, the way to print a diagnostic with an arbitrary string location is simply to call fprintf(stderr, ...).

But presumably you're always printing a location that really came from GCC, so it should work to make Dehydra use location objects that contain the original source_location as well. I'll prepare a patch with this. I'm also going to add a toString() method for locations, which should help preserve current Dehydra usages.

Comment 8

[David Mandelin [:dmandelin]]

Attachment: New patch

Comment 9

[David Mandelin [:dmandelin]]

Attachment: New patch, don't accidentally hit enter this time

OK, this works for Dehydra and Treehydra, with backward-compatible Dehydra string repr on locations, single entry point for diagnostics.

Comment 10

[(dormant account)]

Comment on attachment 319682 [details] [diff] [review]
New patch, don't accidentally hit enter this time

somehow i doubt this is the patch you meant to attach

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Grrr, working patch

Comment 12

[(dormant account)]

Comment on attachment 319699 [details] [diff] [review]
Grrr, working patch

>@@ -305,11 +304,39 @@ void dehydra_setLoc(Dehydra *this, JSObj
> void dehydra_setLoc(Dehydra *this, JSObject *obj, tree t) {
>   location_t loc = location_of (t);
>   if (loc_is_unknown(loc)) return;
>+  /* Don't attach empty locations */
>+  convert_location_t(this, obj, LOC, loc);
>+}

Not sure what the meaning of the comment is here, is it referring to code above it or below. Looks like a leftover.

Other than that seems good. I like convert_tree.js and platform.js changes.

Comment 13

[David Mandelin [:dmandelin]]

Pushed. Yeah, I was pleasantly surprised that the required change to convert_tree.js was actually nice.