Validate that the lint crate uses a compatible `marker_api` version

Context

I spent some time debugging undefined behavior when my lint crate was using the published version 0.1.1 of the marker_api crate, but I used the driver from the latest master built locally. The udnefined behavior was caused by the breaking change in the lint crate ABI.

The ABI of the span function of the DriverContext here was broken in #195.

marker/marker_api/src/context.rs

Line 248 in 878f1b8

pub span: extern "C" fn(&'ast (), SpanId) -> &'ast Span<'ast>,

As a result I was getting a long hang and an output of 97MB of spaces and span underline symbols ^ printed by rustc because the span it returned me contained start/end values with some random junk in them.

Summary

We should benefit from this function and parse its output as a semver version.

marker/marker_api/src/interface.rs

Lines 65 to 67 in 878f1b8

    
           extern "C" fn marker_api_version() -> &'static str { 
        
               $crate::MARKER_API_VERSION 
        
           }

Then marker_adapter may use the semver crate (the one used by cargo) to validate the compatibility of the marker_api that the marker_rustc_driver uses and the marker_api used by the lint crate.

We must ensure we never introduce breaking changes within the compatible updates. Beware that cargo's semver rules are a bit different from the common intuition, or maybe not that different. To be sure, take into account that different minor versions within the 0 major version are not considered compatible. This is done to allow frequent breaking changes for crates that didn't reach the 1.0.0 version yet.

marker_adapter already checks for this. But I didn't increase the version yet, I wasn't sure at first, when I should increase it, and it got lost during the fixes afterwards. I plan to update the version directly after the next release. So I'll release v0.2.0 and then make all crates have the version v0.3.0-dev. We could also check try to also check the last commit hash for dev commits

It would also be good, to override the use marker_api version of the lint crates, to be the same one the driver uses. This can be done with the --config flag when we build the lint crates, like this:--config "dependencies.marker_api='0.0.0'".
`

I see there is a check for the version here, I didn't notice it

marker/marker_adapter/src/loader.rs

Lines 150 to 155 in 6a5f211

    
           let krate_api_version = unsafe { get_api_version() }; 
        
           if krate_api_version != MARKER_API_VERSION { 
        
               return Err(LoadingError::IncompatibleVersion { 
        
                   krate_version: krate_api_version.to_string(), 
        
               }); 
        
           }

It is good but is too restrictive. We may allow version drifts. For example, if we do a patch hotfix then it will break the lint crates if the users don't update their Cargo.toml but they update cargo-marker.

The idea of using marker's config to set the marker API version seems something to look at, but in current formulation it isn't ideal. This would be a surprising behavior if we use some different marker API version for compiling the lint crates other than what these crates set in their [dependencies].

I am sure there are a bunch of similar problems that we have here but with the versioning between cargo-marker, marker_rustc_driver, marker_api, cargo, rustc, rustup. This isn't specific to marker_api only. Each individual point of contact between the components needs a separate review to define the use cases, potential ways for version drifts, and how it should be solved.

I think for now saying that users must make sure their version of marker_api matches with the version of cargo-marker and the driver is reasonable, but only at the very early stage. I see it as a frustrating experience when users try to update their lints to the new version. So eventually this should be solved. I'll try to take a look into this when I finish with some other work in cargo-marker

It is good but is too restrictive. We may allow version drifts. For example, if we do a patch hotfix then it will break the lint crates if the users don't update their Cargo.toml but they update cargo-marker.

Why is it too restrictive? The API version is checked against the driver and cargo-marker also fetches the API version from the driver. The problem is, that any type changes can cause undefined behavior, since they're sent over an FFI bound. In the best case, this will just result in a crash, but it could cause the driver or lint crates to do some wonky stuff. If we keep the public interface stable, we can just override the use marker_api version, as long as the lint crate is using an older on.

Based on what can happen, I might actually say that it might not be restrictive enough.

The idea of using marker's config to set the marker API version seems something to look at, but in current formulation it isn't ideal. This would be a surprising behavior if we use some different marker API version for compiling the lint crates other than what these crates set in their [dependencies].

We should only do this one most of the API is settled. Before that, we might want to read the metadata of the lint crates and warn the user if they use an old version instead.

I am sure there are a bunch of similar problems that we have here but with the versioning between cargo-marker, marker_rustc_driver, marker_api, cargo, rustc, rustup. This isn't specific to marker_api only. Each individual point of contact between the components needs a separate review to define the use cases, potential ways for version drifts, and how it should be solved.

Currently, I have the following rough guideline. The driver is the central component. It defines which API version is used by lint crates and which rustc version is used. cargo-marker is responsible for installation, compilation and driver discovery. If it discovers a driver, it acts on its behalf to compile the lints with the right rustc and api version.

I think for now saying that users must make sure their version of marker_api matches with the version of cargo-marker and the driver is reasonable, but only at the very early stage. I see it as a frustrating experience when users try to update their lints to the new version. So eventually this should be solved. I'll try to take a look into this when I finish with some other work in cargo-marker

Sure, please share your thoughts, if you have any. I've manly look at this from the technical and theoretical standpoint so far :)

Why is it too restrictive?

If we do a patch hotfix then it will break the lint crates if the users don't update their Cargo.toml but they update cargo-marker.
The hotfix means a patch release where we don't break the ABI of the marker_api. For example: to fix some internal bug.

I'm not 100% sure that I understand you correctly. This is an answer from my current guess what you mean, if it doesn't answer the question, could you elaborate a bit further, maybe with an example? 😅

For the hotfix version: If it's only a hotfix for the driver/adapter or cargo-marker, we can bump that version separately and keep the API version lower. The used API version is read from the actual marker_api dependency from the driver. So, in theory we can have driver v0.2.1 for the API version v0.2.0. If we need to do a hotfix for the API, it's very very likely that this will also break the ABI.

Once Marker is more stable and doesn't have API breaking changes anymore (just ABI once and new additions), it should be possible to just override the dependency during lint crate compilation.

	extern "C" fn marker_api_version() -> &'static str {
	$crate::MARKER_API_VERSION
	}

	let krate_api_version = unsafe { get_api_version() };
	if krate_api_version != MARKER_API_VERSION {
	return Err(LoadingError::IncompatibleVersion {
	krate_version: krate_api_version.to_string(),
	});
	}