extract codegen & introduce optparse-applicative

[dannypsnl]

• introduce optparse-applicative to parse CLI input
• extract codegen to simplify program

With optparse, user can get CLI helps

$ witc-exe -h instance
Usage: witc-exe instance COMMAND

  Generate code for instance (wasm)

Available commands:
  import                   Generate import code for instance (wasm)
  export                   Generate export code for instance (wasm)

[dm4]

LGTM, but the CI failed. Should we fix it now?

[dannypsnl]

@dm4 fixed

[dm4]

🆗

[juntao]

Experiment: I will say the magic words to summon the code review bot. Please excuse the construction!

flows summarize

[juntao]

Hello, I am a serverless review bot on flows.network. Here are my reviews of code commits in this PR.

---

Overall, these patches seem to make beneficial changes to the codebase, like improving modularity and consistency, and adding new features. However, there are some potential issues that need to be addressed. For instance, the use of optparse-applicative library and changes to the signature of codegen function may break the existing modules, and the lack of tests leave the codebase vulnerable to potential bugs. Additionally, the check_version function in invoke-witc Rust bindings has a fixed version number, which may break the check when an updated witc-exe is released. Therefore, it needs to be updated with a range of compatible versions or minimum required version.

Details

Commit 1

The patch implements changes that extract codegen from the Main.hs module into a new function codegen. It also introduces the optparse-applicative library to parse command-line arguments. The patch replaces the previous pattern match with conditional code that checks the mode of the command. Finally, the patch implements the instance import, instance export, runtime import, and runtime export commands using the codegen function.

The changes seem to be beneficial because they improve the modularity of the code and make it more flexible. The new codebase also conforms to a well-accepted Haskell library, which should improve its consistency and robustness. However, it's not clear whether this is the best way to introduce optparse-applicative, and the patch doesn't explain why optparse-applicative was chosen over other libraries. Also, the patch is missing tests, which should be added before merging it into the repository.

Commit 2

Key Changes:

• The library optparse-applicative has been added as a new dependency.
• The main function has been refactored to use optparse-applicative for command line argument parsing.
• The old handle function and several child functions have been removed.

Potential Problems:

• There are no potential problems that can be identified by looking at this patch. More context and testing are needed to identify issues that could arise from these changes.
• Consider if optparse-applicative is an appropriate dependency for the project, particularly if the project does not require the advanced command line parsing features this library provides.

Commit 3

Key Changes:

• The code now uses optparse-applicative library for command-line parsing instead of the previous command line parsers (e.g. getArgs, parseCommandLineOptions).
• The functions instanceImport, instanceExport, runtimeImport, runtimeExport and codegen have been removed.
• The codegen function signature has been changed to (Direction -> Side -> FilePath -> String -> IO ()), which reflects that the function now takes arguments in a different order than before.

The use of optparse-applicative library should provide better error handling and a more consistent command-line interface. However, there's a potential problem with the changes in the signature of the codegen function. If the function was being used in more places outside this file, this change would affect other areas of the codebase that used the function. It is possible that some of these use cases might break and require changes. Therefore, a major version bump might be required to signal the API change to the users of the library.

Additionally, there are some minor changes in the code like using value function for default values, changing commands' descriptions, and removing unused functions.

Commit 4

This patch only updates the CHANGELOG by adding three new features to the Unreleased section. The three new features are:

• Improved command line interface with --help and subcommand
• Support for checking imports to ensure type definitions are present
• Wasm interface types support

There are no potential problems identified in this patch as it only adds to the CHANGELOG.

Commit 5

This patch changes the version number of the code. Specifically, it changes the version number "0.2" to "0.2.0". There are no potential problems identified in this patch.

Commit 6

The patch fixes the check_version function in invoke-witc Rust bindings which verifies whether the version of witc-exe is correct. The patch replaces the args(["version"]) with args(["--version"]) which is the correct command for getting the version of witc-exe.

A potential problem with this patch is that it might expect a fixed version number of witc-exe. Whenever witc-exe is updated and a new version is released, the check in check_version may fail. Instead of a fixed version number, the check should use a range of compatible versions, or better yet, parse the version number and compare it with a minimum required version.

[juntao]

Here is another bot that reviews changed code files in a PR. The magic words to summon it are:

flows review

[juntao]

Hello, I am a serverless review bot on flows.network. Here are my reviews of changed source code files in this PR.

---

app/Main.hs

There are no significant problems with this source code snippet.

However, I would suggest a couple of small changes:

1. It would be better to separate versionOption and programOptions out of the main function as separate top-level bindings.
2. Since there is no need to execute anything when check is called, it makes more sense for its return type to be Parser () rather than Parser (IO ()).

Here's the updated code:

module Main where

import Control.Monad
import Data.Functor
import Data.List (isSuffixOf)
import Options.Applicative
import Prettyprinter
import Prettyprinter.Render.Terminal
import System.Directory
import Wit

versionOption :: Parser (a -> a)
versionOption = infoOption "0.2.0" (long "version" <> help "Show version")

programOptions :: Parser (IO ())
programOptions =
  subparser
    ( command
        "check"
        ( info
            (check <$> optional (strArgument (metavar "FILE" <> help "Name of the thing to create")))
            (progDesc "Validate wit file")
        )
        <> command
          "instance"
          ...
      )

check :: Maybe FilePath -> IO ()
check (Just file) = checkFileWithDoneHint file
check Nothing = do
  dir <- getCurrentDirectory
  witFileList <- filter (".wit" `isSuffixOf`) <$> listDirectory dir
  mapM_ checkFileWithDoneHint witFileList

checkFileWithDoneHint :: FilePath -> IO ()
checkFileWithDoneHint file = do
  checkFile file $> ()
  putDoc $ pretty file <+> annotate (color Green) (pretty "OK") <+> line

main :: IO ()
main = do
  join $
    execParser
      ( info
          (helper <*> versionOption <*> programOptions)
          ( fullDesc
              <> progDesc "compiler for wit"
              <> header
                "witc - compiler for wit, a language for describing wasm interface types"
          )
      )

codegen :: Direction -> Side -> FilePath -> String -> IO ()
codegen d s file importName =
  parseFile file
    >>= eitherIO check0
    >>= eitherIO (putDoc . prettyFile Config {language = Rust, direction = d, side = s} importName)

The patch modifies the Main module by refactoring the command-line interface using Options.Applicative. It replaces getArgs with execParser to parse command-line arguments. The handle function is replaced with a check function that takes a Maybe FilePath argument and validates the given file or all the .wit files in the current directory in case of Nothing. Additionally, the codegen function is added to generate code for runtime or instance according to the command-line inputs.

bindings/rust/invoke-witc/src/lib.rs

Overall, the code looks good and there are no obvious security issues from the given source code snippet. However, there are some minor suggestions for improvement:

• The unreachable!() macro is used twice in this code. It may be a better practice to instead return a Result type to deal with unexpected or invalid input.
• Instead of hard-coding the expected version number of witc-exe as "0.2.0", it might be better to retrieve this version from a configuration file, package manifest, or other source which may make it easy and flexible to maintain and update.
• Currently, the code is handling panics with a panic!() macro which simply aborts the program. This might be okay for simple utilities like this code, but for larger systems, panics may not be desirable or even catastrophic. It might be a better practice to use Rust's Result type and return an error message rather than panic.
• There are no comments in the code, it may be helpful to add comments or clear function names to provide additional documentation to make it easier for other developers to understand the code.

The patch updates the arguments to the Command::new method in the check_version function. It changes the argument from version to --version in order to correctly pass the version command to the witc-exe application.

package.yaml

This is a valid package.yaml file defining metadata for a Haskell package, so there are no glaring issues with the file. However, I can provide some general comments:

• It is good to see a clear definition of the package's dependencies and extensions it uses.
• It is also helpful to have specified ghc-options that are reasonably strict, especially Wall and compat.
• It is good practice to add tests to the package, which is done in this file. hspec is a good testing library for Haskell.
• We can also see that optparse-applicative is added to the dependencies list, which suggests that the package might accept command-line arguments.

The patch added optparse-applicative to the dependencies list and modified ghc-options by adding -Wall. The -Wall option turns on a number of warnings for the compiler. The addition of optparse-applicative suggests that the package may now accept command-line arguments.

witc.cabal

This code snippet provides a cabal file for building a Haskell application witc. The dependency optparse-applicative is introduced for command line parsing.

I cannot find any potential problems with this code snippet. However, it is important to note that reviewing only the cabal file is not enough to ensure the code's correctness, security, or maintainability. The actual implementation of the dependencies and the application itself must be reviewed carefully.

The patch introduces a new dependency, optparse-applicative, and adds it to the library, executable, and test-suite sections of the cabal file.