Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[PoC] Injectable source loc arg #7344

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Conversation

zth
Copy link
Collaborator

@zth zth commented Mar 16, 2025

100% inspired by MoonBit's autofill SourceLoc function argument, this makes it possible to inject a labelled argument to any function, and as long as it's optionak, has the type of sourceLocPos and/or sourceLocValuePath (builtin types), and has a default value of %autoFill. Then each call to that function will inject information about the call site, without needing to pass those argument explicitly.

This is useful in many different scenarios, like for example logging (see rescript-logger which does the same, but with a PPX), tests, error reporting, and so on.

Example:
Tst.res

let someFn = (name: string, ~callsiteValuePath: sourceLocValuePath=%autoFill, ~callsitePos: sourceLocPos=%autoFill) => {
  Console.log2(callsiteValuePath, callsitePos)
  "Hello " ++ name
}
// Generated by ReScript, PLEASE EDIT WITH CARE


function someFn(name, callsiteValuePath, callsitePos) {
  console.log(callsiteValuePath, callsitePos);
  return "Hello " + name;
}

export {
  someFn,
}
/* No side effect */

OtherFile.res

module SomeModule = {
  let someName = Tst.someFn("from SomeModule")
}
// Generated by ReScript, PLEASE EDIT WITH CARE

import * as Tst from "./Tst.bs.js";

let someName = Tst.someFn("from SomeModule", "OtherFile.SomeModule.someName", "OtherFile.res;2;17;2;46");

let SomeModule = {
  someName: someName
};

export {
  SomeModule,
}
/* someName Not a pure module */
  • sourceLocValuePath is a string at runtime in the form of Path.To.Module.value. So, if you're MyModule in SomeFile.res, and inside of a let binding called myFunction, you'll get SomeFile.MyModule.myFunction.
  • sourceLocPos is a string at runtime in the form of <filename>;<startLine>;<startCol>;<endLine>;<endCol>.

Both are abstract types, and soon I'll add tools and functions that can be used to work with those abstract types.

Questions

Are what I've added so far enough? Do we want more/less information? Other things to think/worry about?

Things left to figure out

  • Figure out toplevel, which does not work right now
  • Figure out the editor tooling - do we just hide injectable args? Fixed by enforcing an optional arg instead of required.
  • Editor tooling - completions etc for sourceLoc which is a builtin
  • Tests, docs, etc

@zth zth marked this pull request as draft March 16, 2025 20:32
@DZakh
Copy link
Contributor

DZakh commented Mar 17, 2025

This looks like a super useful feature 🔥

But maybe what would be nice is to split the object into separate arguments, so it doesn't needlessly add runtime overhead. If we are talking about logging use-case, I think this is an important point.

Also, for the cases I personally see, I think in the beginning, it would be best to simply include the prepared path to the function call like: src/OtherFile.res:2:17. This will be cheap in terms of runtime performance and useful for logging and test frameworks.

@zth
Copy link
Collaborator Author

zth commented Mar 17, 2025

@DZakh thanks for the feedback! The current object is bloated, I agree, but I don't see where runtime performance would be an issue here...? Could you expand on that part?

More opinions on what to include/not include would be good. cc @cknitt @fhammerschmidt @cometkim @tsnobip

Also, there's an alternative solution where we could just allow all the constants (__FILE__, __MODULE__, __POS__, etc) as separate autofill args. Not sure if that's better. Might be a fair bit more confusing, having just 1 injectable arg is pretty nice because it's contained.

@DZakh
Copy link
Contributor

DZakh commented Mar 17, 2025

These are two object allocations + quite a lot of duplicated strings, which increase the bundle

@cknitt
Copy link
Member

cknitt commented Mar 17, 2025

Awesome work!

I agree with @DZakh. While it can be nice in some situations to get everything in a single object, efficiency-wise it is not ideal, e.g. for a logger.

Personally, for a logger, I would like to be able to obtain the __VALUE_PATH__ as a single argument to be able to match what rescript-logger is doing.

@zth
Copy link
Collaborator Author

zth commented Mar 17, 2025

So, how about this:

  • We select a few of these constants we want to make possible to inject. __VALUE_PATH__ and __POS__ sounds reasonable to me, that should cover most of the use cases
  • Both constants are always injected as strings in JS
  • We make these constants we expose abstract types, like sourceLocValuePath and sourceLocPos. You inject them by providing an arg annotate with any of those abstract types
  • We add a module SourceLoc or similar to Pervasives (or wherever makes sense). That module holds functions for taking the raw strings (hidden behind the abstract types) and turning them into the format people are likely to want at runtime. For __VALUE_PATH__ that's likely an array of strings (split the raw string on .), and for the equivalent of __POS__ it could be an object with {filename: string, startLine: int, startCol: int, endLine: int, endCol: int}

This gives a compact represenation (less bundle bloat, no uneccessary allocation of objects) while preserving the usefulness to the consumer, with the cost of running a function like sourceLocValuePath->SourceLoc.getValuePath.

What do you think?

@zth
Copy link
Collaborator Author

zth commented Mar 17, 2025

I made the change I talked about above, and updated the example accordingly. What do you think @cknitt @DZakh ?

@cometkim
Copy link
Member

More opinions on what to include/not include would be good.

I'm still thinking about it.

Maybe I need to do something similar in rescript-vitest, but without exposing it to the end user interface. I'm not sure if this is actually useful, since the "call site" is inside the library code anyway.

@cometkim
Copy link
Member

One concern:

Bundling error messages or source information from libraries used in the critical path is not a good idea and should be treated as a development-only feature. As we don't have such a flag, as other compilers have a --release flag (I guess MoonBit has such flag?) for stripping dev-only info, and we're not that adaptive like a bundler, users should be careful about optimization themselves.

These are two object allocations + quite a lot of duplicated strings, which increase the bundle

Some critical JS libraries (especially for the Web) have an additional compilation step for similar cases, like invariant.

@cometkim
Copy link
Member

I like the idea of ​​using abstract types to inject metadata, but at least there needs to be an option for the compiler to change its configuration depending on the target environment.

@zth
Copy link
Collaborator Author

zth commented Mar 18, 2025

One concern:

Bundling error messages or source information from libraries used in the critical path is not a good idea and should be treated as a development-only feature. As we don't have such a flag, as other compilers have a --release flag (I guess MoonBit has such flag?) for stripping dev-only info, and we're not that adaptive like a bundler, users should be careful about optimization themselves.

These are two object allocations + quite a lot of duplicated strings, which increase the bundle

Some critical JS libraries (especially for the Web) have an additional compilation step for similar cases, like invariant.

I think for some cases stripping it out of release builds makes sense, but there are plenty of cases where it doesn't (like a logger you use in production). Voiding the string itself in build probably makes the most sense, we can't really change the fn signature depending on build env.

Anyway, I lean towards saying that this is a separate issue since we don't have that type of setup today. For this particular feature we just need to decide whether to include it or not, and if so in which form.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants