Warn when running a deprecated or internal module main function

[Lioncat2002]

This PR closes issue #2569

Features added:

A new is_deprecated field of type Deprecation has been introduced into ModuleFunction struct
updated struct looks like this:

pub struct ModuleFunction {
    pub package: EcoString,
    pub is_deprecated: Deprecation
}

The get_main_function inside of ModuleInterface has been updated to return the deprecation value from the value variable

        Ok(ModuleFunction {
            package: self.package.clone(),
            is_deprecated: value.deprecation.clone(),
        })

A new is_internal function has been introduced into the Built struct. It accepts the module similar to the other functions in the struct, fetches it's ModuleInterface and returns either a bool or an error based on if the module is present in the Hashmap

Added warning when the module being called is either deprecated or internal

 if built.is_internal(&module.clone().into()).is_ok() {
        let warning = Warning::InternalMain;
        warning_emitter.emit(warning);
    }

    // A module can not be run if it does not exist or does not have a public main function.
    let main_function = get_or_suggest_main_function(built, &module, target)?;

    //Warn incase the main function being run has been deprecated
    if main_function.deprecation.is_deprecated() {
        let deprecation_message=match main_function.deprecation{
            gleam_core::type_::Deprecation::Deprecated { message } => Some(message),
            gleam_core::type_::Deprecation::NotDeprecated=>None
        };
        let warning = Warning::DeprecatedMain{message: deprecation_message.unwrap()};
        warning_emitter.emit(warning);
    }

[GearsDatapacks]

I've addressed your questions in the related issue here

[GearsDatapacks]

Looks good so far. Since this PR isn't ready to be merged yet, would you mind converting it to a draft?

[Lioncat2002]

Yeah sure

[Lioncat2002]

PR should be ready for review

[Lioncat2002]

fixed failing workflows

[lpil]

Nice, thank you! I've left some notes inline. Could you also update the changelog file and add a snapshot test for each of the two warnings, so we can see what they look like.

[lpil]

Actually nevermind the snapshot tests, the warnings look simple enough. If you could add the module name to each of them though that would be great. Something like "The module x/y/z is internal" and "The main function in the module x/y/z is internal".

[Lioncat2002]

Yeah sure, will add in the module names

[Lioncat2002]

Module names added

[Lioncat2002]

should I fix these conflicts?

[GearsDatapacks]

At some point yes, although this won't be merged until 1.9 is released.
Make sure to rebase, as merge commits are not allowed in this codebase

[Lioncat2002]

cool

[lpil]

Thank you! I've left some notes inline.

[lpil]

Format this the same as the existing deprecation message please

[Lioncat2002]

I have updated the deprecation text, but run function doesn't have access to the location data like path and src? I can pass the module name itself but I don't think that's correct?

[lpil]

This is using a different warning emitter to build::main. I think we want to only use a single one per-run as we will be tracking things such as the total count when the project denys any warnings.

[Lioncat2002]

ok

[Lioncat2002]

@lpil I fixed it by adding a new parameter in the crate::build::main for the warning emitter
Since currently Rc::new(ConsoleWarningEmitter) was being passed directly
Previous:

pub fn main(paths: &ProjectPaths, options: Options, manifest: Manifest) -> Result<Built> {
    main_with_warnings(paths, options, manifest, Rc::new(ConsoleWarningEmitter))
}

Current:

pub fn main(
    paths: &ProjectPaths,
    options: Options,
    manifest: Manifest,
    warnings: Rc<dyn WarningEmitterIO>, // --> new
) -> Result<Built> {
    main_with_warnings(paths, options, manifest, warnings)
}

is this fine?

[lpil]

When can the module be unknown at this point? Looking at the unwrap_or here.

Don't clone the module please. Build an ecostring beforehand and use that here and below.

[Lioncat2002]

changed modules into a EcoString,
As for the unwrap_or it might happen when the module doesn't exist?

pub fn is_internal(&self, module: &EcoString) -> Result<bool, Error> {
        match self.module_interfaces.get(module) {
            Some(module_data) => Ok(module_data.is_internal),
            None => Err(Error::ModuleDoesNotExist {
                module: module.clone(),
                suggestion: None,
            }),
        }
    }

I had made this method to check if a particular module when passed by name is internal

[lpil]

This is still warning for internal module from the root package!

[Lioncat2002]

I must be misunderstanding a bit,

I am trying to test with

@internal
pub fn main() {
  io.println("hello")
}

marking @internal to the main will be giving the warning right?

can you please give an gleam code showing the case where it is happening?

[lpil]

It would be great to have snapshot tests for how these warnings look please!

[lpil]

Hello @Lioncat2002 ! Are you working on this atm?

[Lioncat2002]

Hi @lpil , sorry, I have been tied up with irl stuff recently.
I think most of the requirements for the fix is done, some snapshot tests are left.
Feel free to reassign this to anyone else wanting to work.