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

Conditional configuration & environment #994

Merged
merged 7 commits into from
Aug 6, 2024
Merged

Conditional configuration & environment #994

merged 7 commits into from
Aug 6, 2024

Conversation

KtorZ
Copy link
Member

@KtorZ KtorZ commented Aug 4, 2024

Context

Closes #937.

Preview

image

☝️ Many things happening on the image above:

  • The project defines a bunch of environments under the env directory.
  • The project defines an extra set of configurations in its aiken.toml
  • Tests make use of the special env and config modules, which are populated based on the --env flag. Defaulting to the environment/config called default when not present.
  • Tests are making conflicting assertions which can only pass in one of the given environment. As illustrated, since --env preprod is passed, only the preprod tests pass.

No default environment?

image

Unknown environment?

Completely unknown Close enough
image image

Missing configuration?

image

Note

This only triggers a warning because it may not be problematic should the configuration be unused. If it is used, we simply fallback to a usual 'unknown module' error.

Changelog

  • 📍 Parse sources of conditional env modules.
    Do nothing about it yet, but trigger an error if env/default.ak is
    missing; but only if there's any module at all under env.

  • 📍 Thread down environment module from cli down to the type-checker
    We simply provide a flag with a free-form output which acts as
    the module to lookup in the 'env' folder. The strategy is to replace
    the environment module name on-the-fly when a user tries to import
    'env'.

    If the environment isn't found, an 'UnknownModule' error is raised
    (which I will slightly adjust in a following commits to something more
    related to environment)

    There are few important consequences to this design which may not seem
    immediately obvious:

    1. We parse and type-check every env modules, even if they aren't
      used. This ensures that code doesn't break with a compilation error
      simply because people forgot to type-check a given env.

      Note that compilation could still fail because the env module
      itself could provide an invalid API. So it only prevents each
      modules to be independently wrong when taken in isolation.

    2. Technically, this also means that one can import env modules in
      other env modules by their names. I don't know if it's a good or
      bad idea at this point but it doesn't really do any wrong;
      dependencies and cycles are handlded all-the-same.

  • 📍 Ensure env modules dependencies are properly handled.
    We figure out dependencies by looking at 'use' definition in parsed
    modules. However, in the case of environment modules, we must consider
    all of them when seeing "use env". Without that, the env modules are
    simply compiled in parallel and may not yet have been compiled when
    they are needed as actual dependencies.

  • 📍 Create dedicated error when environment isn't found.
    This is less confusing that getting an 'UnknownModule' error reporting
    even a different module name than the one actually being important
    ('env').

    Also, this commit fixes a few errors found in the type-checker
    when reporting 'UnknownModule' errors. About half the time, we would
    actually attached imported modules instead of importable modules
    to the error, making the neighboring suggestion quite worse (nay
    useless).

  • 📍 Allow simple expressions as configuration in aiken.toml
    This is currently extremely limited as it only supports (UTF-8)
    bytearrays and integers. We should seek to at least support hex bytes
    sequences, as well as bools, lists and possibly options.

    For the latter, we the rework on constant outlined in Hoist complex constant #992 is
    necessary.

  • 📍 Allow bytes to be defined as plain strings, or with specified encoding.
    The syntax is as follows:

    { "bytes" = "...", "encoding" = "" }

    The following encoding are accepted:

    "utf8", "utf-8", "hex", "base16"

    Note: the duplicates are only there to make it easier for people to
    discover them by accident. When "hex" (resp. "base16") is specified,
    the bytes string will be decoded and must be a valid hex string.

  • 📍 Fill-in CHANGELOG.

KtorZ added 7 commits August 3, 2024 17:42
  Do nothing about it yet, but trigger an error if env/default.ak is
  missing; but only if there's any module at all under env.
  We simply provide a flag with a free-form output which acts as
  the module to lookup in the 'env' folder. The strategy is to replace
  the environment module name on-the-fly when a user tries to import
  'env'.

  If the environment isn't found, an 'UnknownModule' error is raised
  (which I will slightly adjust in a following commits to something more
  related to environment)

  There are few important consequences to this design which may not seem
  immediately obvious:

  1. We parse and type-check every env modules, even if they aren't
     used. This ensures that code doesn't break with a compilation error
     simply because people forgot to type-check a given env.

     Note that compilation could still fail because the env module
     itself could provide an invalid API. So it only prevents each
     modules to be independently wrong when taken in isolation.

  2. Technically, this also means that one can import env modules in
     other env modules by their names. I don't know if it's a good or
     bad idea at this point but it doesn't really do any wrong;
     dependencies and cycles are handlded all-the-same.
  We figure out dependencies by looking at 'use' definition in parsed
  modules. However, in the case of environment modules, we must consider
  all of them when seeing "use env". Without that, the env modules are
  simply compiled in parallel and may not yet have been compiled when
  they are needed as actual dependencies.
  This is less confusing that getting an 'UnknownModule' error reporting
  even a different module name than the one actually being important
  ('env').

  Also, this commit fixes a few errors found in the type-checker
  when reporting 'UnknownModule' errors. About half the time, we would
  actually attached _imported modules_ instead of _importable modules_
  to the error, making the neighboring suggestion quite worse (nay
  useless).
  This is currently extremely limited as it only supports (UTF-8)
  bytearrays and integers. We should seek to at least support hex bytes
  sequences, as well as bools, lists and possibly options.

  For the latter, we the rework on constant outlined in #992 is
  necessary.
  The syntax is as follows:

  { "bytes" = "...", "encoding" = "<encoding>" }

  The following encoding are accepted:

  "utf8", "utf-8", "hex", "base16"

  Note: the duplicates are only there to make it easier for people to
  discover them by accident. When "hex" (resp. "base16") is specified,
  the bytes string will be decoded and must be a valid hex string.
@KtorZ KtorZ requested a review from a team as a code owner August 4, 2024 12:48
Copy link
Member

@rvcas rvcas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, this is so epic

pub type TypedModule = Module<TypeInfo, TypedDefinition>;
pub type UntypedModule = Module<(), UntypedDefinition>;

#[derive(Debug, Copy, Clone, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
pub enum ModuleKind {
Lib,
Validator,
Env,
Config,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very cool

@rvcas
Copy link
Member

rvcas commented Aug 5, 2024

I don't know if you want to merge this yet, seems ready to me but I'll let you click the button.

@KtorZ KtorZ merged commit e6bb13d into main Aug 6, 2024
12 checks passed
@KtorZ KtorZ deleted the conditional-modules branch August 6, 2024 07:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✅ In Next Release
Development

Successfully merging this pull request may close these issues.

Improve developer experience around parameterized validators
2 participants