429770 - Dehydra/Treehydra: Let script know which plugin is running

Status: RESOLVED WORKSFORME

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[David Mandelin [:dmandelin]]

For automated builds, it would simplify things if the script could know which plugin is running. That way, it can conditionally include analysis scripts that rely on Treehydra. So how about adding a new read-only variable:

    (global) this.plugin -- Can be "dehydra" or "treehydra".

Possible flaw here: The implications for user scripts change if we change whether treehydra is a superset of treehydra. We could make things stabler by adding a pair of variables:

    this.dehydra_enabled -- Boolean. Indicates whether dehydra features are enabled.
    this.treehydra_enabled -- Boolean. Indicates whether treehydra features are enabled.

Comment 1

[(dormant account)]

Technically you don't need this. To do a conditional include, all you have to do is 
function process_tree() {
 include("treehydra.js") <-- include guards should prevent this from being included twice.
}

Similar things can be done to get the other features you need using pure js.

Comment 2

[David Mandelin [:dmandelin]]

Attachment: Proposed patch

Comment 3

[(dormant account)]

I highly disapprove of pollutting the "this." namespace with rarely used stuff. I am not convinced that this can't be implemented in pure JS.

I think we need an equivalent of require() but for readonly properties. Perhaps we should just  abuse require() for that? Something like env(), should be used to show GCC version, plugin version, all kinds of exciting stuff.

Comment 4

[David Mandelin [:dmandelin]]

Hmmm, I thought I wrote a comment explaining why the pure JS solution is painful, but I guess I never finished it. Anyway, one specific problem (beyond the general distaste I have for testing what system you're running in by implicitly querying implementation behavior) is that you want to set after_gcc_pass only if you are running in Treehydra, but you have to do that before process_tree is called. That will just barely work, but you'll get warnings about an unknown 'require' argument in dehydra. Another problem is that the design of your libraries starts to depend on whether they're going to be used this way: if they are going to be included inside process_tree, they *can't* 'require' after_gcc_pass. 

So I strongly advocate making it an explicit variable that can be looked up. Moving it out from global 'this' is fine with me, though. How about just an object 'this.env'--I don't see a need to make it a function. The args can go in there too.

Comment 5

[(dormant account)]

(In reply to comment #4)
> Hmmm, I thought I wrote a comment explaining why the pure JS solution is
> painful, but I guess I never finished it. Anyway, one specific problem (beyond
> the general distaste I have for testing what system you're running in by
> implicitly querying implementation behavior) is that you want to set
> after_gcc_pass only if you are running in Treehydra, but you have to do that
> before process_tree is called. That will just barely work, but you'll get
> warnings about an unknown 'require' argument in dehydra. Another problem is
> that the design of your libraries starts to depend on whether they're going to
> be used this way: if they are going to be included inside process_tree, they
> *can't* 'require' after_gcc_pass. 
> 

Ok I get your point. calling things form process_tree is ugly, but you can also check for if(TREE_CODE) or something else defined in treehydra.js

Comment 6

[David Mandelin [:dmandelin]]

(In reply to comment #5)
> Ok I get your point. calling things form process_tree is ugly, but you can also
> check for if(TREE_CODE) or something else defined in treehydra.js

That's still too much browser-detect-like kludgery for me. But how about putting "var treehydra_enabled = true" in treehydra.js. (Notice I even put the "var" for ppl that don't have "let".)
 

Comment 7

[Vlad Sukhoy]

I think everyone has "let" ever since I actually installed JS 1.7. It was JS 1.6 which didn't have them.

Comment 8

[(dormant account)]

(In reply to comment #6)
> (In reply to comment #5)
> > Ok I get your point. calling things form process_tree is ugly, but you can also
> > check for if(TREE_CODE) or something else defined in treehydra.js
> 
> That's still too much browser-detect-like kludgery for me. But how about
> putting "var treehydra_enabled = true" in treehydra.js. (Notice I even put the
> "var" for ppl that don't have "let".)

So you are saying that adding yet another var to treehydra.js for no real reason other than be checked for existence is better than checking for a function that already serves another purpose?

In the spirit of not adding features unless they are required, I claim that the functionality is already there.

Comment 9

[David Mandelin [:dmandelin]]

It's not "no real reason"! Consider the obvious intent of

  if (treehydra_enabled) {
    include('some_analysis.js');
  }

versus the mystery of:

  if (TREE_CODE) {
    include('some_analysis.js');
  }

The latter design is also fragile. If Treehydra changes TREE_CODE or Dehydra gets a TREE_CODE, it breaks. If a script inadvertently contains any of these lines, or includes anything that does, it breaks:

  include('treehydra.js');
  function TREE_CODE ...
  TREE_CODE = ...

A READONLY PERMANENT property or function is free of that problem.

I strongly recommend against 'if (TREE_CODE)' as the test for running under Treehydra.

Comment 10

[(dormant account)]

(In reply to comment #9)
> It's not "no real reason"! Consider the obvious intent of
> 
>   if (treehydra_enabled) {
>     include('some_analysis.js');
>   }
> 
> versus the mystery of:
> 
>   if (TREE_CODE) {
>     include('some_analysis.js');
>   }

The mystery could be solved with a comment //Check for a presence of treehydra libs/plugin

> 
> The latter design is also fragile. If Treehydra changes TREE_CODE or Dehydra
> gets a TREE_CODE, it breaks. If a script inadvertently contains any of these
> lines, or includes anything that does, it breaks:

treehydra will always contain a TREE_CODE

> 
>   include('treehydra.js');
>   function TREE_CODE ...
>   TREE_CODE = ...

Dehydra will never have TREE_CODE in system.js

> 
> A READONLY PERMANENT property or function is free of that problem.
> 
> I strongly recommend against 'if (TREE_CODE)' as the test for running under
> Treehydra.
> 

So you are arguing for an addition of a feature to help a hacky situation. Treehydra and Dehydra are two separate plugins. Multiplexed or the current way, mixing 2 different scripts is convenient but still hacky. 

There only ever needs to be 1 script that uses this hack(either with hardcoded filenames or with the hypothetical commandline parsing):
if (TREE_CODE) {
  include(arg[0]);
} else {
  include(arg[1])
}

I don't see why there should be more than 1 multiplexing script or why we should add a feature to standard libraries to encourage this mixing code for treehydra/dehydra practice.